Re: [PATCH 2/7] drm/amdgpu: rework shadow handling during PD clear
On 2/22/19 10:51 AM, Zeng, Oak wrote:
> See one comment inline [Oak]
>
> Thanks,
> Oak
>
> -Original Message-
> From: amd-gfx On Behalf Of Christian
> König
> Sent: Tuesday, February 19, 2019 8:41 AM
> To: amd-gfx@lists.freedesktop.org
> Subject: [PATCH 2/7] drm/amdgpu: rework shadow handling during PD clear
>
> This way we only deal with the real BO in here.
>
> Signed-off-by: Christian König
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 64 --
> 1 file changed, 39 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 12d51d96491e..3c7b98a758c9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -788,38 +788,58 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device
> *adev,
>
> r = ttm_bo_validate(>tbo, >placement, );
> if (r)
> - goto error;
> + return r;
>
> r = amdgpu_ttm_alloc_gart(>tbo);
> if (r)
> return r;
>
> + if (bo->shadow) {
> + r = ttm_bo_validate(>shadow->tbo, >shadow->placement,
> + );
> + if (r)
> + return r;
> +
> + r = amdgpu_ttm_alloc_gart(>shadow->tbo);
> + if (r)
> + return r;
> +
> + }
> [Oak] I don't have the context what a shadow bo is used for, but is it
> possible that a shadow bo can have it own shadow? If yes, here you want to do
> it in a do...while loop. Also although this rework is performance better as
> you only need one job submission, it makes the codes harder to read. The
> original recursive way is easier to understand.
AFAIK shadow BOs are used only for page tables BOs (system memory
backups for VRAM resident BOs) and cannot have their own shadow BOs.
Andrey
> +
> r = amdgpu_job_alloc_with_ib(adev, 64, );
> if (r)
> - goto error;
> + return r;
>
> - addr = amdgpu_bo_gpu_offset(bo);
> - if (ats_entries) {
> - uint64_t ats_value;
> + while (1) {
> + addr = amdgpu_bo_gpu_offset(bo);
> + if (ats_entries) {
> + uint64_t ats_value;
>
> - ats_value = AMDGPU_PTE_DEFAULT_ATC;
> - if (level != AMDGPU_VM_PTB)
> - ats_value |= AMDGPU_PDE_PTE;
> + ats_value = AMDGPU_PTE_DEFAULT_ATC;
> + if (level != AMDGPU_VM_PTB)
> + ats_value |= AMDGPU_PDE_PTE;
>
> - amdgpu_vm_set_pte_pde(adev, >ibs[0], addr, 0,
> - ats_entries, 0, ats_value);
> - addr += ats_entries * 8;
> - }
> + amdgpu_vm_set_pte_pde(adev, >ibs[0], addr, 0,
> + ats_entries, 0, ats_value);
> + addr += ats_entries * 8;
> + }
>
> - if (entries) {
> - uint64_t value = 0;
> + if (entries) {
> + uint64_t value = 0;
>
> - /* Workaround for fault priority problem on GMC9 */
> - if (level == AMDGPU_VM_PTB && adev->asic_type >= CHIP_VEGA10)
> - value = AMDGPU_PTE_EXECUTABLE;
> + /* Workaround for fault priority problem on GMC9 */
> + if (level == AMDGPU_VM_PTB &&
> + adev->asic_type >= CHIP_VEGA10)
> + value = AMDGPU_PTE_EXECUTABLE;
> +
> + amdgpu_vm_set_pte_pde(adev, >ibs[0], addr, 0,
> + entries, 0, value);
> + }
>
> - amdgpu_vm_set_pte_pde(adev, >ibs[0], addr, 0,
> - entries, 0, value);
> + if (bo->shadow)
> + bo = bo->shadow;
> + else
> + break;
> }
>
> amdgpu_ring_pad_ib(ring, >ibs[0]); @@ -838,16 +858,10 @@ static
> int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
> amdgpu_bo_fence(bo, fence, true);
> dma_fence_put(fence);
>
> - if (bo->shadow)
> - return amdgpu_vm_clear_bo(adev, vm, bo->shadow,
> - level, pte_support_ats);
> -
> return 0;
>
> error_free:
> amdgpu_job_free(job);
> -
> -error:
> return r;
> }
>
> --
> 2.17.1
>
>
RE: [PATCH 2/7] drm/amdgpu: rework shadow handling during PD clear
See one comment inline [Oak]
Thanks,
Oak
-Original Message-
From: amd-gfx On Behalf Of Christian
König
Sent: Tuesday, February 19, 2019 8:41 AM
To: amd-gfx@lists.freedesktop.org
Subject: [PATCH 2/7] drm/amdgpu: rework shadow handling during PD clear
This way we only deal with the real BO in here.
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 64 --
1 file changed, 39 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 12d51d96491e..3c7b98a758c9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -788,38 +788,58 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
r = ttm_bo_validate(>tbo, >placement, );
if (r)
- goto error;
+ return r;
r = amdgpu_ttm_alloc_gart(>tbo);
if (r)
return r;
+ if (bo->shadow) {
+ r = ttm_bo_validate(>shadow->tbo, >shadow->placement,
+ );
+ if (r)
+ return r;
+
+ r = amdgpu_ttm_alloc_gart(>shadow->tbo);
+ if (r)
+ return r;
+
+ }
[Oak] I don't have the context what a shadow bo is used for, but is it possible
that a shadow bo can have it own shadow? If yes, here you want to do it in a
do...while loop. Also although this rework is performance better as you only
need one job submission, it makes the codes harder to read. The original
recursive way is easier to understand.
+
r = amdgpu_job_alloc_with_ib(adev, 64, );
if (r)
- goto error;
+ return r;
- addr = amdgpu_bo_gpu_offset(bo);
- if (ats_entries) {
- uint64_t ats_value;
+ while (1) {
+ addr = amdgpu_bo_gpu_offset(bo);
+ if (ats_entries) {
+ uint64_t ats_value;
- ats_value = AMDGPU_PTE_DEFAULT_ATC;
- if (level != AMDGPU_VM_PTB)
- ats_value |= AMDGPU_PDE_PTE;
+ ats_value = AMDGPU_PTE_DEFAULT_ATC;
+ if (level != AMDGPU_VM_PTB)
+ ats_value |= AMDGPU_PDE_PTE;
- amdgpu_vm_set_pte_pde(adev, >ibs[0], addr, 0,
- ats_entries, 0, ats_value);
- addr += ats_entries * 8;
- }
+ amdgpu_vm_set_pte_pde(adev, >ibs[0], addr, 0,
+ ats_entries, 0, ats_value);
+ addr += ats_entries * 8;
+ }
- if (entries) {
- uint64_t value = 0;
+ if (entries) {
+ uint64_t value = 0;
- /* Workaround for fault priority problem on GMC9 */
- if (level == AMDGPU_VM_PTB && adev->asic_type >= CHIP_VEGA10)
- value = AMDGPU_PTE_EXECUTABLE;
+ /* Workaround for fault priority problem on GMC9 */
+ if (level == AMDGPU_VM_PTB &&
+ adev->asic_type >= CHIP_VEGA10)
+ value = AMDGPU_PTE_EXECUTABLE;
+
+ amdgpu_vm_set_pte_pde(adev, >ibs[0], addr, 0,
+ entries, 0, value);
+ }
- amdgpu_vm_set_pte_pde(adev, >ibs[0], addr, 0,
- entries, 0, value);
+ if (bo->shadow)
+ bo = bo->shadow;
+ else
+ break;
}
amdgpu_ring_pad_ib(ring, >ibs[0]); @@ -838,16 +858,10 @@ static
int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
amdgpu_bo_fence(bo, fence, true);
dma_fence_put(fence);
- if (bo->shadow)
- return amdgpu_vm_clear_bo(adev, vm, bo->shadow,
- level, pte_support_ats);
-
return 0;
error_free:
amdgpu_job_free(job);
-
-error:
return r;
}
--
2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 2/7] drm/amdgpu: rework shadow handling during PD clear
Comments inline.
On 2019-02-19 8:40 a.m., Christian König wrote:
> This way we only deal with the real BO in here.
>
> Signed-off-by: Christian König
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 64 --
> 1 file changed, 39 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 12d51d96491e..3c7b98a758c9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -788,38 +788,58 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device
> *adev,
>
> r = ttm_bo_validate(>tbo, >placement, );
> if (r)
> - goto error;
> + return r;
>
> r = amdgpu_ttm_alloc_gart(>tbo);
> if (r)
> return r;
>
> + if (bo->shadow) {
> + r = ttm_bo_validate(>shadow->tbo, >shadow->placement,
> + );
> + if (r)
> + return r;
> +
> + r = amdgpu_ttm_alloc_gart(>shadow->tbo);
> + if (r)
> + return r;
> +
> + }
> +
> r = amdgpu_job_alloc_with_ib(adev, 64, );
I guess that's still big enough to fit 4 instead of 2 SDMA commands (10
dwords each).
> if (r)
> - goto error;
> + return r;
>
> - addr = amdgpu_bo_gpu_offset(bo);
> - if (ats_entries) {
> - uint64_t ats_value;
> + while (1) {
> + addr = amdgpu_bo_gpu_offset(bo);
> + if (ats_entries) {
> + uint64_t ats_value;
>
> - ats_value = AMDGPU_PTE_DEFAULT_ATC;
> - if (level != AMDGPU_VM_PTB)
> - ats_value |= AMDGPU_PDE_PTE;
> + ats_value = AMDGPU_PTE_DEFAULT_ATC;
> + if (level != AMDGPU_VM_PTB)
> + ats_value |= AMDGPU_PDE_PTE;
>
> - amdgpu_vm_set_pte_pde(adev, >ibs[0], addr, 0,
> - ats_entries, 0, ats_value);
> - addr += ats_entries * 8;
> - }
> + amdgpu_vm_set_pte_pde(adev, >ibs[0], addr, 0,
> + ats_entries, 0, ats_value);
> + addr += ats_entries * 8;
> + }
>
> - if (entries) {
> - uint64_t value = 0;
> + if (entries) {
> + uint64_t value = 0;
>
> - /* Workaround for fault priority problem on GMC9 */
> - if (level == AMDGPU_VM_PTB && adev->asic_type >= CHIP_VEGA10)
> - value = AMDGPU_PTE_EXECUTABLE;
> + /* Workaround for fault priority problem on GMC9 */
> + if (level == AMDGPU_VM_PTB &&
> + adev->asic_type >= CHIP_VEGA10)
> + value = AMDGPU_PTE_EXECUTABLE;
> +
> + amdgpu_vm_set_pte_pde(adev, >ibs[0], addr, 0,
> + entries, 0, value);
> + }
>
> - amdgpu_vm_set_pte_pde(adev, >ibs[0], addr, 0,
> - entries, 0, value);
> + if (bo->shadow)
> + bo = bo->shadow;
> + else
> + break;
> }
Instead of a while(1) endless loop, this could be written as a do ...
while loop with a sane termination condition like this:
do {
...
bo = bo->shadow;
} while (bo);
Assuming that you don't need "bo" after this. You do below, but I think
that's a mistake anyway. See the next comment.
>
> amdgpu_ring_pad_ib(ring, >ibs[0]);
> @@ -838,16 +858,10 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device
> *adev,
> amdgpu_bo_fence(bo, fence, true);
Here you'll only fence the shadow BO.
Regards,
Felix
> dma_fence_put(fence);
>
> - if (bo->shadow)
> - return amdgpu_vm_clear_bo(adev, vm, bo->shadow,
> - level, pte_support_ats);
> -
> return 0;
>
> error_free:
> amdgpu_job_free(job);
> -
> -error:
> return r;
> }
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 2/7] drm/amdgpu: rework shadow handling during PD clear
This way we only deal with the real BO in here.
Signed-off-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 64 --
1 file changed, 39 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 12d51d96491e..3c7b98a758c9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -788,38 +788,58 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
r = ttm_bo_validate(>tbo, >placement, );
if (r)
- goto error;
+ return r;
r = amdgpu_ttm_alloc_gart(>tbo);
if (r)
return r;
+ if (bo->shadow) {
+ r = ttm_bo_validate(>shadow->tbo, >shadow->placement,
+ );
+ if (r)
+ return r;
+
+ r = amdgpu_ttm_alloc_gart(>shadow->tbo);
+ if (r)
+ return r;
+
+ }
+
r = amdgpu_job_alloc_with_ib(adev, 64, );
if (r)
- goto error;
+ return r;
- addr = amdgpu_bo_gpu_offset(bo);
- if (ats_entries) {
- uint64_t ats_value;
+ while (1) {
+ addr = amdgpu_bo_gpu_offset(bo);
+ if (ats_entries) {
+ uint64_t ats_value;
- ats_value = AMDGPU_PTE_DEFAULT_ATC;
- if (level != AMDGPU_VM_PTB)
- ats_value |= AMDGPU_PDE_PTE;
+ ats_value = AMDGPU_PTE_DEFAULT_ATC;
+ if (level != AMDGPU_VM_PTB)
+ ats_value |= AMDGPU_PDE_PTE;
- amdgpu_vm_set_pte_pde(adev, >ibs[0], addr, 0,
- ats_entries, 0, ats_value);
- addr += ats_entries * 8;
- }
+ amdgpu_vm_set_pte_pde(adev, >ibs[0], addr, 0,
+ ats_entries, 0, ats_value);
+ addr += ats_entries * 8;
+ }
- if (entries) {
- uint64_t value = 0;
+ if (entries) {
+ uint64_t value = 0;
- /* Workaround for fault priority problem on GMC9 */
- if (level == AMDGPU_VM_PTB && adev->asic_type >= CHIP_VEGA10)
- value = AMDGPU_PTE_EXECUTABLE;
+ /* Workaround for fault priority problem on GMC9 */
+ if (level == AMDGPU_VM_PTB &&
+ adev->asic_type >= CHIP_VEGA10)
+ value = AMDGPU_PTE_EXECUTABLE;
+
+ amdgpu_vm_set_pte_pde(adev, >ibs[0], addr, 0,
+ entries, 0, value);
+ }
- amdgpu_vm_set_pte_pde(adev, >ibs[0], addr, 0,
- entries, 0, value);
+ if (bo->shadow)
+ bo = bo->shadow;
+ else
+ break;
}
amdgpu_ring_pad_ib(ring, >ibs[0]);
@@ -838,16 +858,10 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
amdgpu_bo_fence(bo, fence, true);
dma_fence_put(fence);
- if (bo->shadow)
- return amdgpu_vm_clear_bo(adev, vm, bo->shadow,
- level, pte_support_ats);
-
return 0;
error_free:
amdgpu_job_free(job);
-
-error:
return r;
}
--
2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
