[PATCH 0/2] fix gmc page fault on navi1X

[PATCH 0/2] fix gmc page fault on navi1X

[xinhui pan]

We hit gmc page fault on navi1X.
UMR tells that the physical address of pte is bad.
Two issues:
1) we did not sync job schedule fence while update mapping.
we sync resv, but the last fence is not there. So any wait on the resv
finished before job completed. say, bo has been released while job
running.
2) we might unref page table bo during update ptes, at the same time, there
is job pending on bo. and submit a job in commit after free bo.
We need free the bo after adding all fence to bo.

xinhui pan (2):
  drm//amdgpu: Always sync fence before unlock eviction_lock
  drm/amdgpu: unref the bo after job submit

 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 46 +++++++++++++++++++-------
 1 file changed, 34 insertions(+), 12 deletions(-)

-- 
2.17.1

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

[PATCH 1/2] drm//amdgpu: Always sync fence before unlock eviction_lock

[xinhui pan]

The fence generated in ->commit is a shared one, so add it to resv.
And we need do that with eviction lock hold.

Currently we only sync last_direct/last_delayed before ->prepare. But we
fail to sync the last fence generated by ->commit. That cuases problems
if eviction happenes later, but it does not sync the last fence.

Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Felix Kuehling <Felix.Kuehling@amd.com>
Signed-off-by: xinhui pan <xinhui.pan@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 9 +++++++--
 1 file changed, 7 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..f424b5969930 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1582,6 +1582,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 	struct amdgpu_vm_update_params params;
 	enum amdgpu_sync_mode sync_mode;
 	int r;
+	struct amdgpu_bo *root = vm->root.base.bo;
 
 	memset(&params, 0, sizeof(params));
 	params.adev = adev;
@@ -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);
 
@@ -1623,6 +1622,12 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 
 	r = vm->update_funcs->commit(&params, fence);
 
+	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);
+
 error_unlock:
 	amdgpu_vm_eviction_unlock(vm);
 	return r;
-- 
2.17.1

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

[PATCH 2/2] drm/amdgpu: unref the bo after job submit

[xinhui pan]

Otherwise we might free BOs before job has completed.
We add the fence to BO after commit, so free BOs after that.

Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Felix Kuehling <Felix.Kuehling@amd.com>
Signed-off-by: xinhui pan <xinhui.pan@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 37 +++++++++++++++++++-------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index f424b5969930..513f3b77d3da 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -942,13 +942,17 @@ static int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
  *
  * @entry: PDE to free
  */
-static void amdgpu_vm_free_table(struct amdgpu_vm_pt *entry)
+static void amdgpu_vm_free_table(struct amdgpu_vm_pt *entry,
+					struct list_head *head)
 {
 	if (entry->base.bo) {
 		entry->base.bo->vm_bo = NULL;
 		list_del(&entry->base.vm_status);
-		amdgpu_bo_unref(&entry->base.bo->shadow);
-		amdgpu_bo_unref(&entry->base.bo);
+		if (!head) {
+			amdgpu_bo_unref(&entry->base.bo->shadow);
+			amdgpu_bo_unref(&entry->base.bo);
+		} else
+			list_add(&entry->base.vm_status, head);
 	}
 	kvfree(entry->entries);
 	entry->entries = NULL;
@@ -965,7 +969,8 @@ static void amdgpu_vm_free_table(struct amdgpu_vm_pt *entry)
  */
 static void amdgpu_vm_free_pts(struct amdgpu_device *adev,
 			       struct amdgpu_vm *vm,
-			       struct amdgpu_vm_pt_cursor *start)
+			       struct amdgpu_vm_pt_cursor *start,
+				   struct list_head *head)
 {
 	struct amdgpu_vm_pt_cursor cursor;
 	struct amdgpu_vm_pt *entry;
@@ -973,10 +978,10 @@ static void amdgpu_vm_free_pts(struct amdgpu_device *adev,
 	vm->bulk_moveable = false;
 
 	for_each_amdgpu_vm_pt_dfs_safe(adev, vm, start, cursor, entry)
-		amdgpu_vm_free_table(entry);
+		amdgpu_vm_free_table(entry, head);
 
 	if (start)
-		amdgpu_vm_free_table(start->entry);
+		amdgpu_vm_free_table(start->entry, head);
 }
 
 /**
@@ -1428,7 +1433,8 @@ static void amdgpu_vm_fragment(struct amdgpu_vm_update_params *params,
  */
 static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
 				 uint64_t start, uint64_t end,
-				 uint64_t dst, uint64_t flags)
+				 uint64_t dst, uint64_t flags,
+				 struct list_head *head)
 {
 	struct amdgpu_device *adev = params->adev;
 	struct amdgpu_vm_pt_cursor cursor;
@@ -1539,7 +1545,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
 			 * completely covered by the range and so potentially still in use.
 			 */
 			while (cursor.pfn < frag_start) {
-				amdgpu_vm_free_pts(adev, params->vm, &cursor);
+				amdgpu_vm_free_pts(adev, params->vm, &cursor, head);
 				amdgpu_vm_pt_next(adev, &cursor);
 			}
 
@@ -1583,6 +1589,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 	enum amdgpu_sync_mode sync_mode;
 	int r;
 	struct amdgpu_bo *root = vm->root.base.bo;
+	struct list_head head;
 
 	memset(&params, 0, sizeof(params));
 	params.adev = adev;
@@ -1590,6 +1597,8 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 	params.direct = direct;
 	params.pages_addr = pages_addr;
 
+	INIT_LIST_HEAD(&head);
+
 	/* Implicitly sync to command submissions in the same VM before
 	 * unmapping. Sync to moving fences before mapping.
 	 */
@@ -1616,7 +1625,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 	if (r)
 		goto error_unlock;
 
-	r = amdgpu_vm_update_ptes(&params, start, last + 1, addr, flags);
+	r = amdgpu_vm_update_ptes(&params, start, last + 1, addr, flags, &head);
 	if (r)
 		goto error_unlock;
 
@@ -1629,6 +1638,14 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
 		amdgpu_bo_fence(root, vm->last_delayed, true);
 
 error_unlock:
+	while (!list_empty(&head)) {
+		struct amdgpu_vm_pt *entry = list_first_entry(&head,
+				struct amdgpu_vm_pt, base.vm_status);
+		list_del(&entry->base.vm_status);
+
+		amdgpu_bo_unref(&entry->base.bo->shadow);
+		amdgpu_bo_unref(&entry->base.bo);
+	}
 	amdgpu_vm_eviction_unlock(vm);
 	return r;
 }
@@ -3123,7 +3140,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 		amdgpu_vm_free_mapping(adev, vm, mapping, NULL);
 	}
 
-	amdgpu_vm_free_pts(adev, vm, NULL);
+	amdgpu_vm_free_pts(adev, vm, NULL, NULL);
 	amdgpu_bo_unreserve(root);
 	amdgpu_bo_unref(&root);
 	WARN_ON(vm->root.base.bo);
-- 
2.17.1

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

Re: [PATCH 1/2] drm//amdgpu: Always sync fence before unlock eviction_lock

[Christian König]

Am 13.03.20 um 08:43 schrieb xinhui pan:
> The fence generated in ->commit is a shared one, so add it to resv.
> And we need do that with eviction lock hold.
>
> Currently we only sync last_direct/last_delayed before ->prepare. But we
> fail to sync the last fence generated by ->commit. That cuases problems
> if eviction happenes later, but it does not sync the last fence.

NAK, that won't work.

We can only add fences when the dma_resv object is locked and that is 
only the case when validating.

I'm considering to just partially revert the patch originally stopping 
to add fences and instead only not add them when invalidating in a 
direct submit.

Christian.

>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
> Signed-off-by: xinhui pan <xinhui.pan@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 9 +++++++--
>   1 file changed, 7 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..f424b5969930 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1582,6 +1582,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>   	struct amdgpu_vm_update_params params;
>   	enum amdgpu_sync_mode sync_mode;
>   	int r;
> +	struct amdgpu_bo *root = vm->root.base.bo;
>   
>   	memset(&params, 0, sizeof(params));
>   	params.adev = adev;
> @@ -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);
>   
> @@ -1623,6 +1622,12 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>   
>   	r = vm->update_funcs->commit(&params, fence);
>   
> +	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);
> +
>   error_unlock:
>   	amdgpu_vm_eviction_unlock(vm);
>   	return r;

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

Re: [PATCH 1/2] drm//amdgpu: Always sync fence before unlock eviction_lock

[Pan, Xinhui]

> 2020年3月13日 16:52，Koenig, Christian <Christian.Koenig@amd.com> 写道：
> 
> Am 13.03.20 um 08:43 schrieb xinhui pan:
>> The fence generated in ->commit is a shared one, so add it to resv.
>> And we need do that with eviction lock hold.
>> 
>> Currently we only sync last_direct/last_delayed before ->prepare. But we
>> fail to sync the last fence generated by ->commit. That cuases problems
>> if eviction happenes later, but it does not sync the last fence.
> 
> NAK, that won't work.
> 
> We can only add fences when the dma_resv object is locked and that is only the case when validating.
> 
well, tha tis true.
but considering this is a PT BO, and only eviction has race on it AFAIK.
as for the individualized resv in bo release, we unref PT BO just after that.
I am still thinking of other races in the real world.

thanks
xinhui

> I'm considering to just partially revert the patch originally stopping to add fences and instead only not add them when invalidating in a direct submit.
> 
> Christian.
> 
>> 
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
>> Signed-off-by: xinhui pan <xinhui.pan@amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 9 +++++++--
>>  1 file changed, 7 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..f424b5969930 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1582,6 +1582,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>>  	struct amdgpu_vm_update_params params;
>>  	enum amdgpu_sync_mode sync_mode;
>>  	int r;
>> +	struct amdgpu_bo *root = vm->root.base.bo;
>>    	memset(&params, 0, sizeof(params));
>>  	params.adev = adev;
>> @@ -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);
>>  @@ -1623,6 +1622,12 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>>    	r = vm->update_funcs->commit(&params, fence);
>>  +	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);
>> +
>>  error_unlock:
>>  	amdgpu_vm_eviction_unlock(vm);
>>  	return r;
> 

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

Re: [PATCH 1/2] drm//amdgpu: Always sync fence before unlock eviction_lock

[Christian König]

Am 13.03.20 um 10:29 schrieb Pan, Xinhui:
>
>> 2020年3月13日 16:52，Koenig, Christian <Christian.Koenig@amd.com> 写道：
>>
>> Am 13.03.20 um 08:43 schrieb xinhui pan:
>>> The fence generated in ->commit is a shared one, so add it to resv.
>>> And we need do that with eviction lock hold.
>>>
>>> Currently we only sync last_direct/last_delayed before ->prepare. But we
>>> fail to sync the last fence generated by ->commit. That cuases problems
>>> if eviction happenes later, but it does not sync the last fence.
>> NAK, that won't work.
>>
>> We can only add fences when the dma_resv object is locked and that is only the case when validating.
>>
> well, tha tis true.
> but considering this is a PT BO, and only eviction has race on it AFAIK.
> as for the individualized resv in bo release, we unref PT BO just after that.
> I am still thinking of other races in the real world.

We should probably just add all pipelined/delayed submissions directly 
to the reservation object in amdgpu_vm_sdma_commit().

Only the direct and invalidating submissions can't be added because we 
can't grab the reservation object in the MMU notifier.

Can you prepare a patch for this?

Regards,
Christian.

>
> thanks
> xinhui
>
>> I'm considering to just partially revert the patch originally stopping to add fences and instead only not add them when invalidating in a direct submit.
>>
>> Christian.
>>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
>>> Signed-off-by: xinhui pan <xinhui.pan@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 9 +++++++--
>>>   1 file changed, 7 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..f424b5969930 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -1582,6 +1582,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>>>   	struct amdgpu_vm_update_params params;
>>>   	enum amdgpu_sync_mode sync_mode;
>>>   	int r;
>>> +	struct amdgpu_bo *root = vm->root.base.bo;
>>>     	memset(&params, 0, sizeof(params));
>>>   	params.adev = adev;
>>> @@ -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);
>>>   @@ -1623,6 +1622,12 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>>>     	r = vm->update_funcs->commit(&params, fence);
>>>   +	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);
>>> +
>>>   error_unlock:
>>>   	amdgpu_vm_eviction_unlock(vm);
>>>   	return r;

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

Re: [PATCH 1/2] drm//amdgpu: Always sync fence before unlock eviction_lock

[Pan, Xinhui]

> 2020年3月13日 17:55，Koenig, Christian <Christian.Koenig@amd.com> 写道：
> 
> Am 13.03.20 um 10:29 schrieb Pan, Xinhui:
>> 
>>> 2020年3月13日 16:52，Koenig, Christian <Christian.Koenig@amd.com> 写道：
>>> 
>>> Am 13.03.20 um 08:43 schrieb xinhui pan:
>>>> The fence generated in ->commit is a shared one, so add it to resv.
>>>> And we need do that with eviction lock hold.
>>>> 
>>>> Currently we only sync last_direct/last_delayed before ->prepare. But we
>>>> fail to sync the last fence generated by ->commit. That cuases problems
>>>> if eviction happenes later, but it does not sync the last fence.
>>> NAK, that won't work.
>>> 
>>> We can only add fences when the dma_resv object is locked and that is only the case when validating.
>>> 
>> well, tha tis true.
>> but considering this is a PT BO, and only eviction has race on it AFAIK.
>> as for the individualized resv in bo release, we unref PT BO just after that.
>> I am still thinking of other races in the real world.
> 
> We should probably just add all pipelined/delayed submissions directly to the reservation object in amdgpu_vm_sdma_commit().
> 
> Only the direct and invalidating submissions can't be added because we can't grab the reservation object in the MMU notifier.
> 
> Can you prepare a patch for this?
> 
yep, I can.
Adding fence to bo resv in every commit introduce a little overload?  As we only need add the last fence to resv given the fact the job scheduer ring is FIFO.
yes,  code should be simple anyway as long as it works.

thanks
xinhui

> Regards,
> Christian.
> 
>> 
>> thanks
>> xinhui
>> 
>>> I'm considering to just partially revert the patch originally stopping to add fences and instead only not add them when invalidating in a direct submit.
>>> 
>>> Christian.
>>> 
>>>> Cc: Christian König <christian.koenig@amd.com>
>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
>>>> Signed-off-by: xinhui pan <xinhui.pan@amd.com>
>>>> ---
>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 9 +++++++--
>>>>  1 file changed, 7 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..f424b5969930 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -1582,6 +1582,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>>>>  	struct amdgpu_vm_update_params params;
>>>>  	enum amdgpu_sync_mode sync_mode;
>>>>  	int r;
>>>> +	struct amdgpu_bo *root = vm->root.base.bo;
>>>>    	memset(&params, 0, sizeof(params));
>>>>  	params.adev = adev;
>>>> @@ -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);
>>>>  @@ -1623,6 +1622,12 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>>>>    	r = vm->update_funcs->commit(&params, fence);
>>>>  +	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);
>>>> +
>>>>  error_unlock:
>>>>  	amdgpu_vm_eviction_unlock(vm);
>>>>  	return r;
> 

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

Re: [PATCH 1/2] drm//amdgpu: Always sync fence before unlock eviction_lock

[Christian König]

Am 13.03.20 um 11:21 schrieb Pan, Xinhui:
>
>> 2020年3月13日 17:55，Koenig, Christian <Christian.Koenig@amd.com> 写道：
>>
>> Am 13.03.20 um 10:29 schrieb Pan, Xinhui:
>>>> 2020年3月13日 16:52，Koenig, Christian <Christian.Koenig@amd.com> 写道：
>>>>
>>>> Am 13.03.20 um 08:43 schrieb xinhui pan:
>>>>> The fence generated in ->commit is a shared one, so add it to resv.
>>>>> And we need do that with eviction lock hold.
>>>>>
>>>>> Currently we only sync last_direct/last_delayed before ->prepare. But we
>>>>> fail to sync the last fence generated by ->commit. That cuases problems
>>>>> if eviction happenes later, but it does not sync the last fence.
>>>> NAK, that won't work.
>>>>
>>>> We can only add fences when the dma_resv object is locked and that is only the case when validating.
>>>>
>>> well, tha tis true.
>>> but considering this is a PT BO, and only eviction has race on it AFAIK.
>>> as for the individualized resv in bo release, we unref PT BO just after that.
>>> I am still thinking of other races in the real world.
>> We should probably just add all pipelined/delayed submissions directly to the reservation object in amdgpu_vm_sdma_commit().
>>
>> Only the direct and invalidating submissions can't be added because we can't grab the reservation object in the MMU notifier.
>>
>> Can you prepare a patch for this?
>>
> yep, I can.
> Adding fence to bo resv in every commit introduce a little overload?

Yes it does, but we used to have this before and it wasn't really 
measurable.

With the unusual exception of mapping really large chunks of fragmented 
system memory we only use one commit for anything <1GB anyway.

Christian.

> As we only need add the last fence to resv given the fact the job scheduer ring is FIFO.
> yes,  code should be simple anyway as long as it works.
>
> thanks
> xinhui
>
>> Regards,
>> Christian.
>>
>>> thanks
>>> xinhui
>>>
>>>> I'm considering to just partially revert the patch originally stopping to add fences and instead only not add them when invalidating in a direct submit.
>>>>
>>>> Christian.
>>>>
>>>>> Cc: Christian König <christian.koenig@amd.com>
>>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>>> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
>>>>> Signed-off-by: xinhui pan <xinhui.pan@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 9 +++++++--
>>>>>   1 file changed, 7 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..f424b5969930 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> @@ -1582,6 +1582,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>>>>>   	struct amdgpu_vm_update_params params;
>>>>>   	enum amdgpu_sync_mode sync_mode;
>>>>>   	int r;
>>>>> +	struct amdgpu_bo *root = vm->root.base.bo;
>>>>>     	memset(&params, 0, sizeof(params));
>>>>>   	params.adev = adev;
>>>>> @@ -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);
>>>>>   @@ -1623,6 +1622,12 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>>>>>     	r = vm->update_funcs->commit(&params, fence);
>>>>>   +	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);
>>>>> +
>>>>>   error_unlock:
>>>>>   	amdgpu_vm_eviction_unlock(vm);
>>>>>   	return r;

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

Re: [PATCH 1/2] drm//amdgpu: Always sync fence before unlock eviction_lock

[Pan, Xinhui]

> 2020年3月13日 18:23，Koenig, Christian <Christian.Koenig@amd.com> 写道：
> 
> Am 13.03.20 um 11:21 schrieb Pan, Xinhui:
>> 
>>> 2020年3月13日 17:55，Koenig, Christian <Christian.Koenig@amd.com> 写道：
>>> 
>>> Am 13.03.20 um 10:29 schrieb Pan, Xinhui:
>>>>> 2020年3月13日 16:52，Koenig, Christian <Christian.Koenig@amd.com> 写道：
>>>>> 
>>>>> Am 13.03.20 um 08:43 schrieb xinhui pan:
>>>>>> The fence generated in ->commit is a shared one, so add it to resv.
>>>>>> And we need do that with eviction lock hold.
>>>>>> 
>>>>>> Currently we only sync last_direct/last_delayed before ->prepare. But we
>>>>>> fail to sync the last fence generated by ->commit. That cuases problems
>>>>>> if eviction happenes later, but it does not sync the last fence.
>>>>> NAK, that won't work.
>>>>> 
>>>>> We can only add fences when the dma_resv object is locked and that is only the case when validating.
>>>>> 
>>>> well, tha tis true.
>>>> but considering this is a PT BO, and only eviction has race on it AFAIK.
>>>> as for the individualized resv in bo release, we unref PT BO just after that.
>>>> I am still thinking of other races in the real world.
>>> We should probably just add all pipelined/delayed submissions directly to the reservation object in amdgpu_vm_sdma_commit().
>>> 
>>> Only the direct and invalidating submissions can't be added because we can't grab the reservation object in the MMU notifier.

wait, I see amdgpu_vm_handle_fault will grab the resv lock of root BO.
so no race then?

>>> 
>>> Can you prepare a patch for this?
>>> 
>> yep, I can.
>> Adding fence to bo resv in every commit introduce a little overload?
> 
> Yes it does, but we used to have this before and it wasn't really measurable.
> 
> With the unusual exception of mapping really large chunks of fragmented system memory we only use one commit for anything <1GB anyway.
> 
> Christian.
> 
>> As we only need add the last fence to resv given the fact the job scheduer ring is FIFO.
>> yes,  code should be simple anyway as long as it works.
>> 
>> thanks
>> xinhui
>> 
>>> Regards,
>>> Christian.
>>> 
>>>> thanks
>>>> xinhui
>>>> 
>>>>> I'm considering to just partially revert the patch originally stopping to add fences and instead only not add them when invalidating in a direct submit.
>>>>> 
>>>>> Christian.
>>>>> 
>>>>>> Cc: Christian König <christian.koenig@amd.com>
>>>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>>>> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
>>>>>> Signed-off-by: xinhui pan <xinhui.pan@amd.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 9 +++++++--
>>>>>>  1 file changed, 7 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..f424b5969930 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>> @@ -1582,6 +1582,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>>>>>>  	struct amdgpu_vm_update_params params;
>>>>>>  	enum amdgpu_sync_mode sync_mode;
>>>>>>  	int r;
>>>>>> +	struct amdgpu_bo *root = vm->root.base.bo;
>>>>>>    	memset(&params, 0, sizeof(params));
>>>>>>  	params.adev = adev;
>>>>>> @@ -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);
>>>>>>  @@ -1623,6 +1622,12 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>>>>>>    	r = vm->update_funcs->commit(&params, fence);
>>>>>>  +	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);
>>>>>> +
>>>>>>  error_unlock:
>>>>>>  	amdgpu_vm_eviction_unlock(vm);
>>>>>>  	return r;
> 

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

Re: [PATCH 1/2] drm//amdgpu: Always sync fence before unlock eviction_lock

[Christian König]

Am 13.03.20 um 11:40 schrieb Pan, Xinhui:
>
>> 2020年3月13日 18:23，Koenig, Christian <Christian.Koenig@amd.com> 写道：
>>
>> Am 13.03.20 um 11:21 schrieb Pan, Xinhui:
>>>> 2020年3月13日 17:55，Koenig, Christian <Christian.Koenig@amd.com> 写道：
>>>>
>>>> Am 13.03.20 um 10:29 schrieb Pan, Xinhui:
>>>>>> 2020年3月13日 16:52，Koenig, Christian <Christian.Koenig@amd.com> 写道：
>>>>>>
>>>>>> Am 13.03.20 um 08:43 schrieb xinhui pan:
>>>>>>> The fence generated in ->commit is a shared one, so add it to resv.
>>>>>>> And we need do that with eviction lock hold.
>>>>>>>
>>>>>>> Currently we only sync last_direct/last_delayed before ->prepare. But we
>>>>>>> fail to sync the last fence generated by ->commit. That cuases problems
>>>>>>> if eviction happenes later, but it does not sync the last fence.
>>>>>> NAK, that won't work.
>>>>>>
>>>>>> We can only add fences when the dma_resv object is locked and that is only the case when validating.
>>>>>>
>>>>> well, tha tis true.
>>>>> but considering this is a PT BO, and only eviction has race on it AFAIK.
>>>>> as for the individualized resv in bo release, we unref PT BO just after that.
>>>>> I am still thinking of other races in the real world.
>>>> We should probably just add all pipelined/delayed submissions directly to the reservation object in amdgpu_vm_sdma_commit().
>>>>
>>>> Only the direct and invalidating submissions can't be added because we can't grab the reservation object in the MMU notifier.
> wait, I see amdgpu_vm_handle_fault will grab the resv lock of root BO.
> so no race then?

No, not in this case.

But Philip, Alejandro and Felix are working on invalidation from an MMU 
callback. And in this case grabbing the lock doesn't work.

But this is a total special use case which can be handled differently.

Christian.

>
>>>> Can you prepare a patch for this?
>>>>
>>> yep, I can.
>>> Adding fence to bo resv in every commit introduce a little overload?
>> Yes it does, but we used to have this before and it wasn't really measurable.
>>
>> With the unusual exception of mapping really large chunks of fragmented system memory we only use one commit for anything <1GB anyway.
>>
>> Christian.
>>
>>> As we only need add the last fence to resv given the fact the job scheduer ring is FIFO.
>>> yes,  code should be simple anyway as long as it works.
>>>
>>> thanks
>>> xinhui
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> thanks
>>>>> xinhui
>>>>>
>>>>>> I'm considering to just partially revert the patch originally stopping to add fences and instead only not add them when invalidating in a direct submit.
>>>>>>
>>>>>> Christian.
>>>>>>
>>>>>>> Cc: Christian König <christian.koenig@amd.com>
>>>>>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>>>>>> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
>>>>>>> Signed-off-by: xinhui pan <xinhui.pan@amd.com>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 9 +++++++--
>>>>>>>   1 file changed, 7 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..f424b5969930 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>>>> @@ -1582,6 +1582,7 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>>>>>>>   	struct amdgpu_vm_update_params params;
>>>>>>>   	enum amdgpu_sync_mode sync_mode;
>>>>>>>   	int r;
>>>>>>> +	struct amdgpu_bo *root = vm->root.base.bo;
>>>>>>>     	memset(&params, 0, sizeof(params));
>>>>>>>   	params.adev = adev;
>>>>>>> @@ -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);
>>>>>>>   @@ -1623,6 +1622,12 @@ static int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
>>>>>>>     	r = vm->update_funcs->commit(&params, fence);
>>>>>>>   +	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);
>>>>>>> +
>>>>>>>   error_unlock:
>>>>>>>   	amdgpu_vm_eviction_unlock(vm);
>>>>>>>   	return r;

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