Re: [PATCH 2/7] drm/amdgpu: rework shadow handling during PD clear

["Kuehling, Felix" <Felix.Kuehling-5C7GfCeVMHo@public.gmane.org>]

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.koenig@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