On 2023-04-26 9:03, Christian König wrote:
Am 25.04.23 um 16:11 schrieb Eric Huang:
Hi Christian,
What do you think about Felix's explanation?
That's unfortunately not something we can do here.
Regards,
Eric
On 2023-04-13 09:28, Felix Kuehling wrote:
Am 2023-04-13 um 07:35 schrieb Christian König:
Am 13.04.23 um 03:01 schrieb Felix Kuehling:
Am 2023-04-12 um 18:25 schrieb Eric Huang:
It is to avoid redundant eviction for KFD's DMAbuf import
bo when dmaunmapping DMAbuf. The DMAbuf import bo has
been set as AMDGPU_PL_PREEMPT in KFD when mapping.
Signed-off-by: Eric Huang <jinhuieric.huang@xxxxxxx>
Reviewed-by: Felix Kuehling <Felix.Kuehling@xxxxxxx>
I'd like to get an Acked-by from Christian as well before
submitting this.
I have to admit that I only partially followed the internal
discussion, but in general you need a *really* good explanation for
this.
E.g. add code comment and explain in the commit message extensively
why this is needed and why there are no alternatives.
OK. I'll give it a shot:
This code path is used among other things when invalidating DMABuf
imports. These imports share a reservation object with the exported
BO. Waiting on all the fences in this reservation will trigger KFD
eviction fences unnecessarily, for example when a DMABuf import for
a DMA mapping on a secondary GPU is being unmapped explicitly. Only
moving the original exported BO requires stopping KFD user mode
queues. If the invalidation is triggered through a move notifier
from the exported BO, then moving the original BO already triggered
the eviction fence and we don't need to wait for it again on the
import.
We can identify DMABuf imports in KFD for secondary GPU DMA mappings
by the mem_type AMDGPU_PL_PREEMPT. In this case, use a wait
operation that ignores KFD eviction fences.
How does this sound?
To be honest like quite a bad idea. Why in the world are imported BOs
moved from GTT to SYSTEM in the first place?
As I understand it, the way to update SG tables in SG BOs (e.g. userptr
and dmabuf imports) is to move them back and forth between system and
GTT domains. If we left the import in the GTT domain all the time, we
would have no way to update it, e.g. after an eviction. Currently the
move to the system domain is done in the unmap code path.
Before memory is freed, we also need to unmap it from GPUVM, including
the DMABuf imports on remote GPUs. For the above reason that currently
includes moving the import to the system domain. If we removed that from
the unmap code path, we'd need to do the move to system somewhere else,
maybe in the mapping/validation path.
The only reason for this I can think of is that the DMA mappings
become invalid for some reasons and in this case waiting for the KFD
fence is actually the absolutely right thing to do.
In this case the reason the only reason for unmapping the memory is that
we're about to free the memory and its DMABuf imports on other GPUs.
This is coming from the application with a promise "I'm no longer
accessing the memory". We don't need to wait for fences here. We only
need to invalidate the PTEs to make sure that any further buggy access
by the application will fault.
Regards,
Felix
Regards,
Christian.
Regards,
Felix
Regards,
Christian.
Thanks,
Felix
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 2430f3e9f3a7..64795fe9eecb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -526,7 +526,12 @@ static int amdgpu_bo_move(struct
ttm_buffer_object *bo, bool evict,
if ((old_mem->mem_type == TTM_PL_TT ||
old_mem->mem_type == AMDGPU_PL_PREEMPT) &&
new_mem->mem_type == TTM_PL_SYSTEM) {
- r = ttm_bo_wait_ctx(bo, ctx);
+ if (old_mem->mem_type == AMDGPU_PL_PREEMPT)
+ r = amdgpu_bo_sync_wait(abo,
+ AMDGPU_FENCE_OWNER_KFD,
+ ctx->interruptible);
+ else
+ r = ttm_bo_wait_ctx(bo, ctx);
if (r)
return r;
