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

[Pan, Xinhui]

> 2020年3月13日 22:13，Koenig, Christian <Christian.Koenig at amd.com> 写道：
> 
> 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 at amd.com>
>> Cc: Alex Deucher <alexander.deucher at amd.com>
>> Cc: Felix Kuehling <Felix.Kuehling at amd.com>
>> Suggested-by: Christian König <christian.koenig at amd.com>
>> Signed-off-by: xinhui pan <xinhui.pan at amd.com>
>> ---
>>  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.
> 

yep, make sense.

> 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);
> }
> 

I think we still need udpate the last_delayed. looks like eviction and some other ioctls test this field to determine  the vm state.
this should be done by a new patch if possible.

> Regards,
> Christian.
> 
>> +		dma_resv_add_shared_fence(p->resv, f);
>> +
>>  	if (fence && !p->direct)
>>  		swap(*fence, f);
>>  	dma_fence_put(f);
>