[PATCH] drm/amdgpu: fix VM sync with always valid BOs v2

[zhoucm1]

NAK, as Roger pointed, the performance of Vulkan Dota2 drops from 72fps 
to 24fps.

This patches will kill Per-vm-BO advantages, even drop to 1/3 of 
previous non-per-vm-bo.

all moved and relocated fence have already synced, we just need to catch 
the va mapping but which is CS itself required, as my proposal in 
another thread, we maybe need to expand va_ioctl to return mapping fence 
to UMD, then UMD passes it to CS as dependency.


Regards,

David Zhou


On 2017年09月11日 15:53, Christian König wrote:
> From: Christian König <christian.koenig at amd.com>
>
> All users of a VM must always wait for updates with always
> valid BOs to be completed.
>
> v2: remove debugging leftovers, rename struct member
>
> Signed-off-by: Christian König <christian.koenig at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 10 ++++++----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 15 ++++++++++-----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  2 +-
>   3 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 8aa37e0..4681dcc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -752,10 +752,6 @@ static int amdgpu_bo_vm_update_pte(struct amdgpu_cs_parser *p)
>   	if (r)
>   		return r;
>   
> -	r = amdgpu_sync_fence(adev, &p->job->sync, vm->last_dir_update);
> -	if (r)
> -		return r;
> -
>   	r = amdgpu_vm_clear_freed(adev, vm, NULL);
>   	if (r)
>   		return r;
> @@ -810,6 +806,12 @@ static int amdgpu_bo_vm_update_pte(struct amdgpu_cs_parser *p)
>   	}
>   
>   	r = amdgpu_vm_handle_moved(adev, vm, &p->job->sync);
> +	if (r)
> +		return r;
> +
> +	r = amdgpu_sync_fence(adev, &p->job->sync, vm->last_update);
> +	if (r)
> +		return r;
>   
>   	if (amdgpu_vm_debug && p->bo_list) {
>   		/* Invalidate all BOs to test for userspace bugs */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 55f1ecb..5042f09 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1140,9 +1140,8 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   				goto error_free;
>   
>   			amdgpu_bo_fence(parent->base.bo, fence, true);
> -			dma_fence_put(vm->last_dir_update);
> -			vm->last_dir_update = dma_fence_get(fence);
> -			dma_fence_put(fence);
> +			dma_fence_put(vm->last_update);
> +			vm->last_update = fence;
>   		}
>   	}
>   
> @@ -1803,6 +1802,12 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
>   			trace_amdgpu_vm_bo_mapping(mapping);
>   	}
>   
> +	if (bo_va->base.bo &&
> +	    bo_va->base.bo->tbo.resv == vm->root.base.bo->tbo.resv) {
> +		dma_fence_put(vm->last_update);
> +		vm->last_update = dma_fence_get(bo_va->last_pt_update);
> +	}
> +
>   	return 0;
>   }
>   
> @@ -2586,7 +2591,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   			 vm->use_cpu_for_update ? "CPU" : "SDMA");
>   	WARN_ONCE((vm->use_cpu_for_update & !amdgpu_vm_is_large_bar(adev)),
>   		  "CPU update of VM recommended only for large BAR system\n");
> -	vm->last_dir_update = NULL;
> +	vm->last_update = NULL;
>   
>   	flags = AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS |
>   			AMDGPU_GEM_CREATE_VRAM_CLEARED;
> @@ -2692,7 +2697,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   	}
>   
>   	amdgpu_vm_free_levels(&vm->root);
> -	dma_fence_put(vm->last_dir_update);
> +	dma_fence_put(vm->last_update);
>   	for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
>   		amdgpu_vm_free_reserved_vmid(adev, vm, i);
>   }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index c1accd1..cb6a622 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -140,7 +140,7 @@ struct amdgpu_vm {
>   
>   	/* contains the page directory */
>   	struct amdgpu_vm_pt     root;
> -	struct dma_fence	*last_dir_update;
> +	struct dma_fence	*last_update;
>   
>   	/* protecting freed */
>   	spinlock_t		freed_lock;