Am 13.03.20 um 11:40 schrieb Pan, Xinhui:
2020年3月13日 18:23，Koenig, Christian <Christian.Koenig@xxxxxxx> 写道：
Am 13.03.20 um 11:21 schrieb Pan, Xinhui:
2020年3月13日 17:55，Koenig, Christian <Christian.Koenig@xxxxxxx> 写道：
Am 13.03.20 um 10:29 schrieb Pan, Xinhui:
2020年3月13日 16:52，Koenig, Christian <Christian.Koenig@xxxxxxx> 写道：
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@xxxxxxx>
Cc: Alex Deucher <alexander.deucher@xxxxxxx>
Cc: Felix Kuehling <Felix.Kuehling@xxxxxxx>
Signed-off-by: xinhui pan <xinhui.pan@xxxxxxx>
---
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(¶ms, 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(¶ms, 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
