Re: [Nouveau] [PATCH 1/3] drm/nouveau: wait for moving fence after pinning

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jun 21, 2021 at 5:53 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Jun 21, 2021 at 5:49 PM Christian König
> <ckoenig.leichtzumerken@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@amd.com>
> > >> CC: stable@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?

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.

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.
-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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau