Re: [PATCH v5] drm_amdgpu: Add job fence to resv conditionally

[Christian König <christian.koenig@xxxxxxx>]

Am 16.03.20 um 13:04 schrieb xinhui pan:
> Job fence on page table should be a shared one, so add it to the root
> page talbe bo resv.
> last_delayed field is not needed anymore. so remove it.
>
> Cc: Christian König <christian.koenig@xxxxxxx>
> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> Cc: Felix Kuehling <Felix.Kuehling@xxxxxxx>
> Suggested-by: Christian König <christian.koenig@xxxxxxx>
> Signed-off-by: xinhui pan <xinhui.pan@xxxxxxx>

One coding style nit pick below.

> ---
> change from v4:
> remove last_delayed filed.
>
> change from v3:
> use vm to get root bo resv.
>
> change from v2:
> use the correct page table bo resv
>
> change from v1:
> fix rebase issue.
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      | 16 ++--------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  1 -
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 10 +++++-----
>   3 files changed, 7 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 73398831196f..9e1baa360e6a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1608,9 +1608,6 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>   
>   		if (!dma_fence_is_signaled(vm->last_direct))
>   			amdgpu_bo_fence(root, vm->last_direct, true);
> -
> -		if (!dma_fence_is_signaled(vm->last_delayed))
> -			amdgpu_bo_fence(root, vm->last_delayed, true);
>   	}
>   
>   	r = vm->update_funcs->prepare(&params, resv, sync_mode);
> @@ -2592,8 +2589,7 @@ bool amdgpu_vm_evictable(struct amdgpu_bo *bo)
>   		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)) {
> +	if (!dma_fence_is_signaled(bo_base->vm->last_direct)) {
>   		amdgpu_vm_eviction_unlock(bo_base->vm);
>   		return false;
>   	}
> @@ -2770,11 +2766,7 @@ long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long 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);
> +	return dma_fence_wait_timeout(vm->last_direct, true, timeout);
>   }
>   
>   /**
> @@ -2847,7 +2839,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		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();
>   
>   	mutex_init(&vm->eviction_lock);
>   	vm->evicting = false;
> @@ -2902,7 +2893,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   
>   error_free_delayed:
>   	dma_fence_put(vm->last_direct);
> -	dma_fence_put(vm->last_delayed);
>   	drm_sched_entity_destroy(&vm->delayed);
>   
>   error_free_direct:
> @@ -3105,8 +3095,6 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   
>   	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) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index b5705fcfc935..b52efc591160 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -279,7 +279,6 @@ struct amdgpu_vm {
>   
>   	/* Last submission to the scheduler entities */
>   	struct dma_fence	*last_direct;
> -	struct dma_fence	*last_delayed;
>   
>   	unsigned int		pasid;
>   	/* dedicated to vm */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> index 4cc7881f438c..15bb30df9ae1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> @@ -104,12 +104,12 @@ static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
>   	if (r)
>   		goto error;
>   
> -	tmp = dma_fence_get(f);
> -	if (p->direct)
> +	if (p->direct) {
> +		tmp = dma_fence_get(f);
>   		swap(p->vm->last_direct, tmp);
> -	else
> -		swap(p->vm->last_delayed, tmp);
> -	dma_fence_put(tmp);
> +		dma_fence_put(tmp);
> +	} else
> +		dma_resv_add_shared_fence(p->vm->root.base.bo->tbo.base.resv, f);

When one part of the if/else has {} the other should as well. In other 
words use "} else {"... here.

Apart from that the patch is Reviewed-by: Christian König 
<christian.koenig@xxxxxxx>.

Thanks,
Christian.

>   
>   	if (fence && !p->direct)
>   		swap(*fence, f);

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx