| Message ID | 20250107055846.536589-1-airlied@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nouveau/fence: handle cross device fences properly. | expand |
On 7/1/25 15:58, Dave Airlie wrote: > From: Dave Airlie <airlied@redhat.com> > > If we have two nouveau controlled devices and one passes a dma-fence > to the other, when we hit the sync path it can cause the second device > to try and put a sync wait in it's pushbuf for the seqno of the context > on the first device. > > Since fence contexts are vmm bound, check the if vmm's match between > both users, this should ensure that fence seqnos don't get used wrongly > on incorrect channels. > > This seems to happen fairly spuriously and I found it tracking down > a multi-card regression report, that seems to work by luck before this. > > Signed-off-by: Dave Airlie <airlied@redhat.com> > Cc: stable@vger.kernel.org Reviewed-by: Ben Skeggs <bskeggs@nvidia.com> > --- > drivers/gpu/drm/nouveau/nouveau_fence.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c > index ee5e9d40c166f..5743c82f4094b 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c > @@ -370,7 +370,8 @@ nouveau_fence_sync(struct nouveau_bo *nvbo, struct nouveau_channel *chan, > > rcu_read_lock(); > prev = rcu_dereference(f->channel); > - if (prev && (prev == chan || > + if (prev && (prev->vmm == chan->vmm) && > + (prev == chan || > fctx->sync(f, prev, chan) == 0)) > must_wait = false; > rcu_read_unlock();
On Tue, Jan 07, 2025 at 03:58:46PM +1000, Dave Airlie wrote: > From: Dave Airlie <airlied@redhat.com> > > If we have two nouveau controlled devices and one passes a dma-fence > to the other, when we hit the sync path it can cause the second device > to try and put a sync wait in it's pushbuf for the seqno of the context > on the first device. > > Since fence contexts are vmm bound, check the if vmm's match between > both users, this should ensure that fence seqnos don't get used wrongly > on incorrect channels. The fence sequence number is global, i.e. per device, hence checking the vmm context seems too restrictive. Wouldn't it be better to ensure that `prev->cli->drm == chan->cli->drm`? This way we can still optimize where dependencies are between different applications, but on the same device. > > This seems to happen fairly spuriously and I found it tracking down > a multi-card regression report, that seems to work by luck before this. > > Signed-off-by: Dave Airlie <airlied@redhat.com> > Cc: stable@vger.kernel.org > --- > drivers/gpu/drm/nouveau/nouveau_fence.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c > index ee5e9d40c166f..5743c82f4094b 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c > @@ -370,7 +370,8 @@ nouveau_fence_sync(struct nouveau_bo *nvbo, struct nouveau_channel *chan, > > rcu_read_lock(); > prev = rcu_dereference(f->channel); > - if (prev && (prev == chan || > + if (prev && (prev->vmm == chan->vmm) && > + (prev == chan || Maybe better break it down a bit, e.g. bool local = prev && (prev->... == chan->...); if (local && ...) { ... } > fctx->sync(f, prev, chan) == 0)) > must_wait = false; > rcu_read_unlock(); > -- > 2.43.0 >
On Wed, 8 Jan 2025 at 02:02, Danilo Krummrich <dakr@kernel.org> wrote: > > On Tue, Jan 07, 2025 at 03:58:46PM +1000, Dave Airlie wrote: > > From: Dave Airlie <airlied@redhat.com> > > > > If we have two nouveau controlled devices and one passes a dma-fence > > to the other, when we hit the sync path it can cause the second device > > to try and put a sync wait in it's pushbuf for the seqno of the context > > on the first device. > > > > Since fence contexts are vmm bound, check the if vmm's match between > > both users, this should ensure that fence seqnos don't get used wrongly > > on incorrect channels. > > The fence sequence number is global, i.e. per device, hence checking the vmm > context seems too restrictive. > > Wouldn't it be better to ensure that `prev->cli->drm == chan->cli->drm`? Can you prove that? I thought the same and I've gone around a few times yesterday/today and convinced myself what I wrote is right. dma_fence_init gets passed the seqno which comes from fctx->sequence, which is nouveau_fence_chan, which gets allocated for each channel. So we should hit this path if we have 2 userspace submits, one with say graphics, the one with copy engine contexts, otherwise we should wait on the CPU. > > drivers/gpu/drm/nouveau/nouveau_fence.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c > > index ee5e9d40c166f..5743c82f4094b 100644 > > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c > > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c > > @@ -370,7 +370,8 @@ nouveau_fence_sync(struct nouveau_bo *nvbo, struct nouveau_channel *chan, > > > > rcu_read_lock(); > > prev = rcu_dereference(f->channel); > > - if (prev && (prev == chan || > > + if (prev && (prev->vmm == chan->vmm) && > > + (prev == chan || > > Maybe better break it down a bit, e.g. > > bool local = prev && (prev->... == chan->...); > > if (local && ...) { > ... > } I'll update that once we resolve the above. Dave.
On Wed, Jan 08, 2025 at 11:04:21AM +1000, Dave Airlie wrote: > On Wed, 8 Jan 2025 at 02:02, Danilo Krummrich <dakr@kernel.org> wrote: > > > > On Tue, Jan 07, 2025 at 03:58:46PM +1000, Dave Airlie wrote: > > > From: Dave Airlie <airlied@redhat.com> > > > > > > If we have two nouveau controlled devices and one passes a dma-fence > > > to the other, when we hit the sync path it can cause the second device > > > to try and put a sync wait in it's pushbuf for the seqno of the context > > > on the first device. > > > > > > Since fence contexts are vmm bound, check the if vmm's match between > > > both users, this should ensure that fence seqnos don't get used wrongly > > > on incorrect channels. > > > > The fence sequence number is global, i.e. per device, hence checking the vmm > > context seems too restrictive. > > > > Wouldn't it be better to ensure that `prev->cli->drm == chan->cli->drm`? > > Can you prove that? I thought the same and I've gone around a few > times yesterday/today and convinced myself what I wrote is right. Honestly, I thought you were implying that by the commit summary and message, but that's more the how you found this. With that bias grep made me end up at pre-nv84 code, where this is actually still the case (see nv17_fence_sync()). But of course for later GPUs it's a per fence-context / channel seqno; can't know what the firmware scheduler puts first. I think we should change the commit message to "handle cross cli fences properly" (channels of the same cli share the cli's vmm) and clarify in the commit message that not only cross device cases are affected. I'd also put that the problem is that (for nv84 and later) we otherwise take the channel ID of the fence' channel and add it on top of the fence-context vma address of the target channel, which (if they have different VMMs) makes us end up with a wrong synchronization point [1]. Cross device could even be worse with very old GPUs, since ->sync() just assumes the same fence-context type between the channels. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/nouveau/nv84_fence.c#n100 > > dma_fence_init gets passed the seqno which comes from fctx->sequence, > which is nouveau_fence_chan, which gets allocated for each channel. > > So we should hit this path if we have 2 userspace submits, one with > say graphics, the one with copy engine contexts, otherwise we should > wait on the CPU. > > > > drivers/gpu/drm/nouveau/nouveau_fence.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c > > > index ee5e9d40c166f..5743c82f4094b 100644 > > > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c > > > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c > > > @@ -370,7 +370,8 @@ nouveau_fence_sync(struct nouveau_bo *nvbo, struct nouveau_channel *chan, > > > > > > rcu_read_lock(); > > > prev = rcu_dereference(f->channel); > > > - if (prev && (prev == chan || > > > + if (prev && (prev->vmm == chan->vmm) && > > > + (prev == chan || > > > > Maybe better break it down a bit, e.g. > > > > bool local = prev && (prev->... == chan->...); > > > > if (local && ...) { > > ... > > } > > I'll update that once we resolve the above. > > Dave.
diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c index ee5e9d40c166f..5743c82f4094b 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fence.c +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c @@ -370,7 +370,8 @@ nouveau_fence_sync(struct nouveau_bo *nvbo, struct nouveau_channel *chan, rcu_read_lock(); prev = rcu_dereference(f->channel); - if (prev && (prev == chan || + if (prev && (prev->vmm == chan->vmm) && + (prev == chan || fctx->sync(f, prev, chan) == 0)) must_wait = false; rcu_read_unlock();