Am 22.06.21 um 11:20 schrieb Daniel Vetter:
On Mon, Jun 21, 2021 at 5:53 PM Daniel Vetter <daniel@xxxxxxxx> wrote:
On Mon, Jun 21, 2021 at 5:49 PM Christian König
<ckoenig.leichtzumerken@xxxxxxxxx> wrote:
Am 21.06.21 um 16:54 schrieb Daniel Vetter:
On Mon, Jun 21, 2021 at 03:03:26PM +0200, Christian König wrote:
We actually need to wait for the moving fence after pinning
the BO to make sure that the pin is completed.
Signed-off-by: Christian König <christian.koenig@xxxxxxx>
CC: stable@xxxxxxxxxx
---
drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c
index 347488685f74..591738545eba 100644
--- a/drivers/gpu/drm/nouveau/nouveau_prime.c
+++ b/drivers/gpu/drm/nouveau/nouveau_prime.c
@@ -93,7 +93,13 @@ int nouveau_gem_prime_pin(struct drm_gem_object *obj)
if (ret)
return -EINVAL;
- return 0;
+ if (nvbo->bo.moving) {
Don't we need to hold the dma_resv to read this? We can grab a reference
and then unlock, but I think just unlocked wait can go boom pretty easily
(since we don't hold a reference or lock so someone else can jump in and
free the moving fence).
The moving fence is only modified while the BO is moved and since we
have just successfully pinned it....
Yeah ... so probably correct, but really tricky. Just wrapping a
ttm_bo_reserve/unreserve around the code you add should be enough and
get the job done?
I think you distracted me a bit with the "it can't move", so yes
there's a guarantee that no other fence can show up in ttm_bo->moving
and confuse us. But it could get set to NULL because someone realized
it signalled. We're not doing that systematically, but relying on
fences never getting garbage-collected for correctness isn't great.
Yeah, that's what I essentially meant with it would be better in general
to take the lock.
Sot the ttm_bo_reserve/unreserve is definitely needed here around this
bit of code. You don't need to merge it with the reserve/unreserve in
the pin function though, it's just to protect against the
use-after-free.
Ah, yes good point. That means I don't need to change the pin/unpin
functions in nouveau at all.
BTW: What do you think of making dma_fence_is_signaled() and
dma_fence_wait_timeout() save to passing in NULL as fence?
I think we have a lot of cases where we check "!fence ||
dma_fence_is_signaled(fence)" or similar.
Christian.
-Daniel
But in general I agree that it would be better to avoid this. I just
didn't wanted to open a bigger can of worms by changing nouveau so much.
Yeah, but I'm kinda thinking of some helpers to wait for the move
fence (so that later on we can switch from having the exclusive fence
to the move fence do that, maybe). And then locking checks in there
would be nice.
Also avoids the case of explaining why lockless here is fine, but
lockless wait for the exclusive fence in e.g. a dynami dma-buf
importer is very much not fine at all. Just all around less trouble.
-Daniel
Christian.
-Daniel
+ ret = dma_fence_wait(nvbo->bo.moving, true);
+ if (ret)
+ nouveau_bo_unpin(nvbo);
+ }
+
+ return ret;
}
void nouveau_gem_prime_unpin(struct drm_gem_object *obj)
--
2.25.1
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
