Re: [PATCH v3 1/2] drm/amdgpu: Add job fence to resv conditionally

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

Am 13.03.20 um 15:07 schrieb xinhui pan:
> Provide resv as a parameter for vm sdma commit.
> Job fence on page table should be a shared one, so add it to the resv.
>
> 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>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      | 4 ++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      | 5 +++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 7 +++++++
>   3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 73398831196f..809ca6e8f40f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1579,6 +1579,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>   				       dma_addr_t *pages_addr,
>   				       struct dma_fence **fence)
>   {
> +	struct amdgpu_bo *root = vm->root.base.bo;
>   	struct amdgpu_vm_update_params params;
>   	enum amdgpu_sync_mode sync_mode;
>   	int r;
> @@ -1604,8 +1605,6 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>   	}
>   
>   	if (flags & AMDGPU_PTE_VALID) {
> -		struct amdgpu_bo *root = vm->root.base.bo;
> -
>   		if (!dma_fence_is_signaled(vm->last_direct))
>   			amdgpu_bo_fence(root, vm->last_direct, true);
>   
> @@ -1613,6 +1612,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>   			amdgpu_bo_fence(root, vm->last_delayed, true);
>   	}
>   
> +	params.resv = root->tbo.base.resv;
>   	r = vm->update_funcs->prepare(&params, resv, sync_mode);
>   	if (r)
>   		goto error_unlock;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index b5705fcfc935..ca6021b4200b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -226,6 +226,11 @@ struct amdgpu_vm_update_params {
>   	 * @num_dw_left: number of dw left for the IB
>   	 */
>   	unsigned int num_dw_left;
> +
> +	/**
> +	 * @resv: sync the resv and add job fence to it conditionally.
> +	 */
> +	struct dma_resv *resv;
>   };
>   
>   struct amdgpu_vm_update_funcs {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> index 4cc7881f438c..a1b270a4da8e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> @@ -111,6 +111,13 @@ static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
>   		swap(p->vm->last_delayed, tmp);
>   	dma_fence_put(tmp);
>   
> +	/* add job fence to resv.
> +	 * MM notifier path is an exception as we can not grab the
> +	 * resv lock.
> +	 */
> +	if (!p->direct && p->resv)

You can just use p->vm->root.base.bo->tbo.base.resv here, no need for a 
new field in the paramater structure.

And it would probably be best to also remove the vm->last_delayed field 
entirely.

In other words use something like this here

if (p->direct) {
    tmp = dma_fence_get(f);
    swap(p->vm->last_direct, tmp);
    dma_fence_put(tmp);
} else {
dma_resv_add_shared_fence(p->vm->root.base.bo->tbo.base.resv, f);
}

Regards,
Christian.

> +		dma_resv_add_shared_fence(p->resv, f);
> +
>   	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