Am 25.05.21 um 17:23 schrieb Daniel Vetter:
On Tue, May 25, 2021 at 5:05 PM Christian König
<ckoenig.leichtzumerken@xxxxxxxxx> wrote:
Hi Daniel,
Am 25.05.21 um 15:05 schrieb Daniel Vetter:
Hi Christian,
On Sat, May 22, 2021 at 10:30:19AM +0200, Christian König wrote:
Am 21.05.21 um 20:31 schrieb Daniel Vetter:
This works by adding the fence of the last eviction DMA operation to BOs
when their backing store is newly allocated. That's what the
ttm_bo_add_move_fence() function you stumbled over is good for: https://elixir.bootlin.com/linux/v5.13-rc2/source/drivers/gpu/drm/ttm/ttm_bo.c#L692
Now the problem is it is possible that the application is terminated before
it can complete it's command submission. But since resource management only
waits for the shared fences when there are some there is a chance that we
free up memory while it is still in use.
Hm where is this code? Would help in my audit that I wanted to do this
week? If you look at most other places like
drm_gem_fence_array_add_implicit() I mentioned earlier, then we don't
treat the shared fences special and always also include the exclusive one.
See amdgpu_gem_object_close():
...
fence = dma_resv_get_excl(bo->tbo.base.resv);
if (fence) {
amdgpu_bo_fence(bo, fence, true);
fence = NULL;
}
...
We explicitly added that because resource management of some other
driver was going totally bananas without that.
But I'm not sure which one that was. Maybe dig a bit in the git and
mailing history of that.
Hm I looked and it's
commit 82c416b13cb7d22b96ec0888b296a48dff8a09eb
Author: Christian König <christian.koenig@xxxxxxx>
Date: Thu Mar 12 12:03:34 2020 +0100
drm/amdgpu: fix and cleanup amdgpu_gem_object_close v4
That sounded more like amdgpu itself needing this, not another driver?
No, that patch was just a follow up moving the functionality around.
But looking at amdgpu_vm_bo_update_mapping() it seems to pick the
right fencing mode for gpu pte clearing, so I'm really not sure what
the bug was that you worked around here?The implementation boils down
to amdgpu_sync_resv() which syncs for the exclusive fence, always. And
there's nothing else that I could find in public history at least, no
references to bug reports or anything. I think you need to dig
internally, because as-is I'm not seeing the problem here.
Or am I missing something here?
See the code here for example:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/nouveau/nouveau_fence.c#L361
Nouveau assumes that when a shared fence is present it doesn't need to
wait for the exclusive one because the shared are always supposed to
finish after the exclusive one.
But for page table unmap fences that isn't true and we ran into a really
nasty and hard to reproduce bug because of this.
I think it would be much more defensive if we could say that we always
wait for the exclusive fence and fix the use case in nouveau and double
check if somebody else does stuff like that as well.
Christian.
-Daniel
