Re: [PATCH 3/5] drm/amdgpu: stop adding VM updates fences to the resv obj

[Felix Kuehling <felix.kuehling@xxxxxxx>]

On 2019-12-05 8:39 a.m., Christian König wrote:
> Don't add the VM update fences to the resv object and remove
> the handling to stop implicitely syncing to them.
>
> Ongoing updates prevent page tables from being evicted and we manually
> block for all updates to complete before releasing PDs and PTS.
>
> This way we can do updates even without the resv obj locked.
>
> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c    |  6 ++---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      | 30 ++++++++++++++++++---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  4 +++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 11 +++++---
>   4 files changed, 40 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> index f1e5fbef54d8..ae8bc766215c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> @@ -243,10 +243,8 @@ int amdgpu_sync_resv(struct amdgpu_device *adev,
>   			/* VM updates are only interesting
>   			 * for other VM updates and moves.

Thanks for updating the commit description. I think this comment should 
also be updated because "other VM updates" fences are no longer in the 
resv. Something like this: VM updates only sync with moves but not with 
user command submissions or KFD evictions fences. With that fixed, this 
patch is

Reviewed-by: Felix Kuehling <Felix.Kuehling@xxxxxxx>

>   			 */
> -			if ((owner != AMDGPU_FENCE_OWNER_UNDEFINED) &&
> -			    (fence_owner != AMDGPU_FENCE_OWNER_UNDEFINED) &&
> -			    ((owner == AMDGPU_FENCE_OWNER_VM) !=
> -			     (fence_owner == AMDGPU_FENCE_OWNER_VM)))
> +			if (owner == AMDGPU_FENCE_OWNER_VM &&
> +			    fence_owner != AMDGPU_FENCE_OWNER_UNDEFINED)
>   				continue;
>   
>   			/* Ignore fence from the same owner and explicit one as
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index a22bd57129d1..0d700e8154c4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -562,8 +562,8 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
>   {
>   	entry->priority = 0;
>   	entry->tv.bo = &vm->root.base.bo->tbo;
> -	/* One for the VM updates, one for TTM and one for the CS job */
> -	entry->tv.num_shared = 3;
> +	/* One for TTM and one for the CS job */
> +	entry->tv.num_shared = 2;
>   	entry->user_pages = NULL;
>   	list_add(&entry->tv.head, validated);
>   }
> @@ -2522,6 +2522,11 @@ bool amdgpu_vm_evictable(struct amdgpu_bo *bo)
>   	if (!dma_resv_test_signaled_rcu(bo->tbo.base.resv, true))
>   		return false;
>   
> +	/* Don't evict VM page tables while they are updated */
> +	if (!dma_fence_is_signaled(bo_base->vm->last_direct) ||
> +	    !dma_fence_is_signaled(bo_base->vm->last_delayed))
> +		return false;
> +
>   	return true;
>   }
>   
> @@ -2687,8 +2692,16 @@ void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint32_t min_vm_size,
>    */
>   long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout)
>   {
> -	return dma_resv_wait_timeout_rcu(vm->root.base.bo->tbo.base.resv,
> -						   true, true, timeout);
> +	timeout = dma_resv_wait_timeout_rcu(vm->root.base.bo->tbo.base.resv,
> +					    true, true, timeout);
> +	if (timeout <= 0)
> +		return timeout;
> +
> +	timeout = dma_fence_wait_timeout(vm->last_direct, true, timeout);
> +	if (timeout <= 0)
> +		return timeout;
> +
> +	return dma_fence_wait_timeout(vm->last_delayed, true, timeout);
>   }
>   
>   /**
> @@ -2757,6 +2770,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   	else
>   		vm->update_funcs = &amdgpu_vm_sdma_funcs;
>   	vm->last_update = NULL;
> +	vm->last_direct = dma_fence_get_stub();
> +	vm->last_delayed = dma_fence_get_stub();
>   
>   	amdgpu_vm_bo_param(adev, vm, adev->vm_manager.root_level, false, &bp);
>   	if (vm_context == AMDGPU_VM_CONTEXT_COMPUTE)
> @@ -2807,6 +2822,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   	vm->root.base.bo = NULL;
>   
>   error_free_delayed:
> +	dma_fence_put(vm->last_direct);
> +	dma_fence_put(vm->last_delayed);
>   	drm_sched_entity_destroy(&vm->delayed);
>   
>   error_free_direct:
> @@ -3007,6 +3024,11 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   		vm->pasid = 0;
>   	}
>   
> +	dma_fence_wait(vm->last_direct, false);
> +	dma_fence_put(vm->last_direct);
> +	dma_fence_wait(vm->last_delayed, false);
> +	dma_fence_put(vm->last_delayed);
> +
>   	list_for_each_entry_safe(mapping, tmp, &vm->freed, list) {
>   		if (mapping->flags & AMDGPU_PTE_PRT && prt_fini_needed) {
>   			amdgpu_vm_prt_fini(adev, vm);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index db561765453b..d93ea9ad879e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -269,6 +269,10 @@ struct amdgpu_vm {
>   	struct drm_sched_entity	direct;
>   	struct drm_sched_entity	delayed;
>   
> +	/* Last submission to the scheduler entities */
> +	struct dma_fence	*last_direct;
> +	struct dma_fence	*last_delayed;
> +
>   	unsigned int		pasid;
>   	/* dedicated to vm */
>   	struct amdgpu_vmid	*reserved_vmid[AMDGPU_MAX_VMHUBS];
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> index 50f487666977..19b7f80758f1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> @@ -95,11 +95,10 @@ static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
>   static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
>   				 struct dma_fence **fence)
>   {
> -	struct amdgpu_bo *root = p->vm->root.base.bo;
>   	struct amdgpu_ib *ib = p->job->ibs;
>   	struct drm_sched_entity *entity;
> +	struct dma_fence *f, *tmp;
>   	struct amdgpu_ring *ring;
> -	struct dma_fence *f;
>   	int r;
>   
>   	entity = p->direct ? &p->vm->direct : &p->vm->delayed;
> @@ -112,7 +111,13 @@ static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
>   	if (r)
>   		goto error;
>   
> -	amdgpu_bo_fence(root, f, true);
> +	tmp = dma_fence_get(f);
> +	if (p->direct)
> +		swap(p->vm->last_direct, tmp);
> +	else
> +		swap(p->vm->last_delayed, tmp);
> +	dma_fence_put(tmp);
> +
>   	if (fence && !p->direct)
>   		swap(*fence, f);
>   	dma_fence_put(f);
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx