On Fri, 26 Jan 2024 at 04:28, Daniel Vetter <daniel@xxxxxxxx> wrote: > > On Tue, Jan 23, 2024 at 05:25:38PM +1000, Dave Airlie wrote: > > From: Dave Airlie <airlied@xxxxxxxxxx> > > > > fences are signalled on nvidia hw using non-stall interrupts. > > > > non-stall interrupts are not latched from my reading. > > > > When nouveau emits a fence, it requests a NON_STALL signalling, > > but it only calls the interface to allow the non-stall irq to happen > > after it has already emitted the fence. A recent change > > eacabb546271 ("nouveau: push event block/allowing out of the fence context") > > made this worse by pushing out the fence allow/block to a workqueue. > > > > However I can't see how this could ever work great, since when > > enable signalling is called, the semaphore has already been emitted > > to the ring, and the hw could already have tried to set the bits, > > but it's been masked off. Changing the allowed mask later won't make > > the interrupt get called again. > > > > For now rip all of this out. > > > > This fixes a bunch of stalls seen running VK CTS sync tests. > > > > Signed-off-by: Dave Airlie <airlied@xxxxxxxxxx> > > --- > > drivers/gpu/drm/nouveau/nouveau_fence.c | 77 +++++-------------------- > > drivers/gpu/drm/nouveau/nouveau_fence.h | 2 - > > 2 files changed, 16 insertions(+), 63 deletions(-) > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c > > index 5057d976fa57..d6d50cdccf75 100644 > > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c > > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c > > @@ -50,24 +50,14 @@ nouveau_fctx(struct nouveau_fence *fence) > > return container_of(fence->base.lock, struct nouveau_fence_chan, lock); > > } > > > > -static int > > +static void > > nouveau_fence_signal(struct nouveau_fence *fence) > > { > > - int drop = 0; > > - > > dma_fence_signal_locked(&fence->base); > > list_del(&fence->head); > > rcu_assign_pointer(fence->channel, NULL); > > > > - if (test_bit(DMA_FENCE_FLAG_USER_BITS, &fence->base.flags)) { > > - struct nouveau_fence_chan *fctx = nouveau_fctx(fence); > > - > > - if (atomic_dec_and_test(&fctx->notify_ref)) > > - drop = 1; > > - } > > - > > dma_fence_put(&fence->base); > > - return drop; > > } > > > > static struct nouveau_fence * > > @@ -93,8 +83,7 @@ nouveau_fence_context_kill(struct nouveau_fence_chan *fctx, int error) > > if (error) > > dma_fence_set_error(&fence->base, error); > > > > - if (nouveau_fence_signal(fence)) > > - nvif_event_block(&fctx->event); > > + nouveau_fence_signal(fence); > > } > > fctx->killed = 1; > > spin_unlock_irqrestore(&fctx->lock, flags); > > @@ -103,8 +92,8 @@ nouveau_fence_context_kill(struct nouveau_fence_chan *fctx, int error) > > void > > nouveau_fence_context_del(struct nouveau_fence_chan *fctx) > > { > > - cancel_work_sync(&fctx->allow_block_work); > > nouveau_fence_context_kill(fctx, 0); > > + nvif_event_block(&fctx->event); > > nvif_event_dtor(&fctx->event); > > fctx->dead = 1; > > > > @@ -127,11 +116,10 @@ nouveau_fence_context_free(struct nouveau_fence_chan *fctx) > > kref_put(&fctx->fence_ref, nouveau_fence_context_put); > > } > > > > -static int > > +static void > > nouveau_fence_update(struct nouveau_channel *chan, struct nouveau_fence_chan *fctx) > > { > > struct nouveau_fence *fence; > > - int drop = 0; > > u32 seq = fctx->read(chan); > > > > while (!list_empty(&fctx->pending)) { > > @@ -140,10 +128,8 @@ nouveau_fence_update(struct nouveau_channel *chan, struct nouveau_fence_chan *fc > > if ((int)(seq - fence->base.seqno) < 0) > > break; > > > > - drop |= nouveau_fence_signal(fence); > > + nouveau_fence_signal(fence); > > } > > - > > - return drop; > > } > > > > static int > > @@ -160,26 +146,13 @@ nouveau_fence_wait_uevent_handler(struct nvif_event *event, void *repv, u32 repc > > > > fence = list_entry(fctx->pending.next, typeof(*fence), head); > > chan = rcu_dereference_protected(fence->channel, lockdep_is_held(&fctx->lock)); > > - if (nouveau_fence_update(chan, fctx)) > > - ret = NVIF_EVENT_DROP; > > + nouveau_fence_update(chan, fctx); > > } > > spin_unlock_irqrestore(&fctx->lock, flags); > > > > return ret; > > } > > > > -static void > > -nouveau_fence_work_allow_block(struct work_struct *work) > > -{ > > - struct nouveau_fence_chan *fctx = container_of(work, struct nouveau_fence_chan, > > - allow_block_work); > > - > > - if (atomic_read(&fctx->notify_ref) == 0) > > - nvif_event_block(&fctx->event); > > - else > > - nvif_event_allow(&fctx->event); > > -} > > - > > void > > nouveau_fence_context_new(struct nouveau_channel *chan, struct nouveau_fence_chan *fctx) > > { > > @@ -191,7 +164,6 @@ nouveau_fence_context_new(struct nouveau_channel *chan, struct nouveau_fence_cha > > } args; > > int ret; > > > > - INIT_WORK(&fctx->allow_block_work, nouveau_fence_work_allow_block); > > INIT_LIST_HEAD(&fctx->flip); > > INIT_LIST_HEAD(&fctx->pending); > > spin_lock_init(&fctx->lock); > > @@ -216,6 +188,12 @@ nouveau_fence_context_new(struct nouveau_channel *chan, struct nouveau_fence_cha > > &args.base, sizeof(args), &fctx->event); > > > > WARN_ON(ret); > > + > > + /* > > + * Always allow non-stall irq events - previously this code tried to > > + * enable/disable them, but that just seems racy as nonstall irqs are unlatched. > > + */ > > + nvif_event_allow(&fctx->event); > > } > > > > int > > @@ -247,8 +225,7 @@ nouveau_fence_emit(struct nouveau_fence *fence) > > return -ENODEV; > > } > > > > - if (nouveau_fence_update(chan, fctx)) > > - nvif_event_block(&fctx->event); > > + nouveau_fence_update(chan, fctx); > > > > list_add_tail(&fence->head, &fctx->pending); > > spin_unlock_irq(&fctx->lock); > > @@ -271,8 +248,8 @@ nouveau_fence_done(struct nouveau_fence *fence) > > > > spin_lock_irqsave(&fctx->lock, flags); > > chan = rcu_dereference_protected(fence->channel, lockdep_is_held(&fctx->lock)); > > - if (chan && nouveau_fence_update(chan, fctx)) > > - nvif_event_block(&fctx->event); > > + if (chan) > > + nouveau_fence_update(chan, fctx); > > spin_unlock_irqrestore(&fctx->lock, flags); > > } > > return dma_fence_is_signaled(&fence->base); > > @@ -530,32 +507,10 @@ static const struct dma_fence_ops nouveau_fence_ops_legacy = { > > .release = nouveau_fence_release > > }; > > > > -static bool nouveau_fence_enable_signaling(struct dma_fence *f) > > -{ > > - struct nouveau_fence *fence = from_fence(f); > > - struct nouveau_fence_chan *fctx = nouveau_fctx(fence); > > - bool ret; > > - bool do_work; > > - > > - if (atomic_inc_return(&fctx->notify_ref) == 0) > > - do_work = true; > > - > > - ret = nouveau_fence_no_signaling(f); > > - if (ret) > > - set_bit(DMA_FENCE_FLAG_USER_BITS, &fence->base.flags); > > - else if (atomic_dec_and_test(&fctx->notify_ref)) > > - do_work = true; > > - > > - if (do_work) > > - schedule_work(&fctx->allow_block_work); > > - > > - return ret; > > -} > > - > > static const struct dma_fence_ops nouveau_fence_ops_uevent = { > > .get_driver_name = nouveau_fence_get_get_driver_name, > > .get_timeline_name = nouveau_fence_get_timeline_name, > > - .enable_signaling = nouveau_fence_enable_signaling, > > + .enable_signaling = nouveau_fence_no_signaling, > > I think you can rip nouveau_fence_no_signaling out too, it doesn't do > anything more than what the signalling codepath does too. > > But maybe separate path since maybe this makes an existing leak more of a > sieve, but it really should be an existing one since you cannot assume > that someone external will ever look at whether your fence is signalled or > not. > -Sima > I think it might be overkill to rip this out, but the fix I put in 6.7 is also having bad side effects, so I'm going to try and revert that and fix that problem first. I think I'd like to keep this irq handling stuff as it seems to matter, but I think the atomic in fctx is wrongly handled and it's a case of misusing atomics instead of locks and I'm going to spend next week considering it in a bit more depth. Dave.