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 <christian.koe...@amd.com>
> ---
> 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(&bo->tbo, &bo->placement, &ctx);
> if (r)
> - goto error;
> + return r;
>
> r = amdgpu_ttm_alloc_gart(&bo->tbo);
> if (r)
> return r;
>
> + if (bo->shadow) {
> + r = ttm_bo_validate(&bo->shadow->tbo, &bo->shadow->placement,
> + &ctx);
> + if (r)
> + return r;
> +
> + r = amdgpu_ttm_alloc_gart(&bo->shadow->tbo);
> + if (r)
> + return r;
> +
> + }
> +
> r = amdgpu_job_alloc_with_ib(adev, 64, &job);
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, &job->ibs[0], addr, 0,
> - ats_entries, 0, ats_value);
> - addr += ats_entries * 8;
> - }
> + amdgpu_vm_set_pte_pde(adev, &job->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, &job->ibs[0], addr, 0,
> + entries, 0, value);
> + }
>
> - amdgpu_vm_set_pte_pde(adev, &job->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, &job->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
