Re: [PATCH 3/7] drm/amdgpu: let amdgpu_vm_clear_bo figure out ats status

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

On 2019-02-19 8:40 a.m., Christian König wrote:
> Instead of providing it from outside figure out the ats status in the
> function itself from the data structures.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>

One suggestion inline. Other than that this patch is Reviewed-by: Felix 
Kuehling <Felix.Kuehling@amd.com>

Regards,
   Felix


> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 52 ++++++++++++++------------
>   1 file changed, 29 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 3c7b98a758c9..48da4ac76837 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -747,8 +747,6 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm)
>    * @adev: amdgpu_device pointer
>    * @vm: VM to clear BO from
>    * @bo: BO to clear
> - * @level: level this BO is at
> - * @pte_support_ats: indicate ATS support from PTE
>    *
>    * Root PD needs to be reserved when calling this.
>    *
> @@ -756,10 +754,11 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm)
>    * 0 on success, errno otherwise.
>    */
>   static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
> -			      struct amdgpu_vm *vm, struct amdgpu_bo *bo,
> -			      unsigned level, bool pte_support_ats)
> +			      struct amdgpu_vm *vm,
> +			      struct amdgpu_bo *bo)
>   {
>   	struct ttm_operation_ctx ctx = { true, false };
> +	unsigned level = adev->vm_manager.root_level;
>   	struct dma_fence *fence = NULL;
>   	unsigned entries, ats_entries;
>   	struct amdgpu_ring *ring;
> @@ -768,17 +767,32 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
>   	int r;
>   
>   	entries = amdgpu_bo_size(bo) / 8;
> +	if (vm->pte_support_ats) {
> +		ats_entries = amdgpu_vm_level_shift(adev, level);
> +		ats_entries += AMDGPU_GPU_PAGE_SHIFT;
> +		ats_entries = AMDGPU_GMC_HOLE_START >> ats_entries;
>   
> -	if (pte_support_ats) {
> -		if (level == adev->vm_manager.root_level) {
> -			ats_entries = amdgpu_vm_level_shift(adev, level);
> -			ats_entries += AMDGPU_GPU_PAGE_SHIFT;
> -			ats_entries = AMDGPU_GMC_HOLE_START >> ats_entries;
> +		if (!bo->parent) {
>   			ats_entries = min(ats_entries, entries);
>   			entries -= ats_entries;
>   		} else {
> -			ats_entries = entries;
> -			entries = 0;
> +			struct amdgpu_bo *ancestor = bo;
> +			struct amdgpu_vm_pt *pt;
> +
> +			++level;
> +			while (ancestor->parent->parent) {
> +				ancestor = ancestor->parent;
> +				++level;
> +			}

This could be simplified as

	do {
		ancestor = ancestor->parent;
		++level;
	} while (ancestor->parent);


> +
> +			pt = container_of(ancestor->vm_bo, struct amdgpu_vm_pt,
> +					  base);
> +			if ((pt - vm->root.entries) >= ats_entries) {
> +				ats_entries = 0;
> +			} else {
> +				ats_entries = entries;
> +				entries = 0;
> +			}
>   		}
>   	} else {
>   		ats_entries = 0;
> @@ -911,7 +925,6 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>   {
>   	struct amdgpu_vm_pt_cursor cursor;
>   	struct amdgpu_bo *pt;
> -	bool ats = false;
>   	uint64_t eaddr;
>   	int r;
>   
> @@ -921,9 +934,6 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>   
>   	eaddr = saddr + size - 1;
>   
> -	if (vm->pte_support_ats)
> -		ats = saddr < AMDGPU_GMC_HOLE_START;
> -
>   	saddr /= AMDGPU_GPU_PAGE_SIZE;
>   	eaddr /= AMDGPU_GPU_PAGE_SIZE;
>   
> @@ -972,7 +982,7 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>   
>   		amdgpu_vm_bo_base_init(&entry->base, vm, pt);
>   
> -		r = amdgpu_vm_clear_bo(adev, vm, pt, cursor.level, ats);
> +		r = amdgpu_vm_clear_bo(adev, vm, pt);
>   		if (r)
>   			goto error_free_pt;
>   	}
> @@ -3047,9 +3057,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   
>   	amdgpu_vm_bo_base_init(&vm->root.base, vm, root);
>   
> -	r = amdgpu_vm_clear_bo(adev, vm, root,
> -			       adev->vm_manager.root_level,
> -			       vm->pte_support_ats);
> +	r = amdgpu_vm_clear_bo(adev, vm, root);
>   	if (r)
>   		goto error_unreserve;
>   
> @@ -3144,9 +3152,8 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm, uns
>   	 * changing any other state, in case it fails.
>   	 */
>   	if (pte_support_ats != vm->pte_support_ats) {
> -		r = amdgpu_vm_clear_bo(adev, vm, vm->root.base.bo,
> -			       adev->vm_manager.root_level,
> -			       pte_support_ats);
> +		vm->pte_support_ats = pte_support_ats;
> +		r = amdgpu_vm_clear_bo(adev, vm, vm->root.base.bo);
>   		if (r)
>   			goto free_idr;
>   	}
> @@ -3154,7 +3161,6 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm, uns
>   	/* Update VM state */
>   	vm->use_cpu_for_update = !!(adev->vm_manager.vm_update_mode &
>   				    AMDGPU_VM_USE_CPU_FOR_COMPUTE);
> -	vm->pte_support_ats = pte_support_ats;
>   	DRM_DEBUG_DRIVER("VM update mode is %s\n",
>   			 vm->use_cpu_for_update ? "CPU" : "SDMA");
>   	WARN_ONCE((vm->use_cpu_for_update && !amdgpu_gmc_vram_full_visible(&adev->gmc)),
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx