Christian König
2021-Jun-21 15:49 UTC
[Nouveau] [PATCH 1/3] drm/nouveau: wait for moving fence after pinning
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 at amd.com> >> CC: stable at kernel.org >> --- >> 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.... 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. 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
2021-Jun-21 15:53 UTC
[Nouveau] [PATCH 1/3] drm/nouveau: wait for moving fence after pinning
On Mon, Jun 21, 2021 at 5:49 PM Christian K?nig <ckoenig.leichtzumerken at gmail.com> 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 at amd.com> > >> CC: stable at kernel.org > >> --- > >> 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?> 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