All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] nouveau: rip out fence irq allow/block sequences.
@ 2024-01-23  7:25 Dave Airlie
  2024-01-25 18:22 ` Daniel Vetter
  0 siblings, 1 reply; 3+ messages in thread
From: Dave Airlie @ 2024-01-23  7:25 UTC (permalink / raw)
  To: dri-devel; +Cc: nouveau

From: Dave Airlie <airlied@redhat.com>

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@redhat.com>
---
 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,
 	.signaled = nouveau_fence_is_signaled,
 	.release = nouveau_fence_release
 };
diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.h b/drivers/gpu/drm/nouveau/nouveau_fence.h
index 28f5cf013b89..380bb0397ed2 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fence.h
+++ b/drivers/gpu/drm/nouveau/nouveau_fence.h
@@ -46,8 +46,6 @@ struct nouveau_fence_chan {
 	char name[32];
 
 	struct nvif_event event;
-	struct work_struct allow_block_work;
-	atomic_t notify_ref;
 	int dead, killed;
 };
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] nouveau: rip out fence irq allow/block sequences.
  2024-01-23  7:25 [PATCH] nouveau: rip out fence irq allow/block sequences Dave Airlie
@ 2024-01-25 18:22 ` Daniel Vetter
  2024-01-25 21:55   ` Dave Airlie
  0 siblings, 1 reply; 3+ messages in thread
From: Daniel Vetter @ 2024-01-25 18:22 UTC (permalink / raw)
  To: Dave Airlie; +Cc: nouveau, dri-devel

On Tue, Jan 23, 2024 at 05:25:38PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> 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@redhat.com>
> ---
>  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

>  	.signaled = nouveau_fence_is_signaled,
>  	.release = nouveau_fence_release
>  };
> diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.h b/drivers/gpu/drm/nouveau/nouveau_fence.h
> index 28f5cf013b89..380bb0397ed2 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_fence.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_fence.h
> @@ -46,8 +46,6 @@ struct nouveau_fence_chan {
>  	char name[32];
>  
>  	struct nvif_event event;
> -	struct work_struct allow_block_work;
> -	atomic_t notify_ref;
>  	int dead, killed;
>  };
>  
> -- 
> 2.43.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] nouveau: rip out fence irq allow/block sequences.
  2024-01-25 18:22 ` Daniel Vetter
@ 2024-01-25 21:55   ` Dave Airlie
  0 siblings, 0 replies; 3+ messages in thread
From: Dave Airlie @ 2024-01-25 21:55 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: nouveau, dri-devel

On Fri, 26 Jan 2024 at 04:28, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Jan 23, 2024 at 05:25:38PM +1000, Dave Airlie wrote:
> > From: Dave Airlie <airlied@redhat.com>
> >
> > 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@redhat.com>
> > ---
> >  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.

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-01-25 21:57 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-23  7:25 [PATCH] nouveau: rip out fence irq allow/block sequences Dave Airlie
2024-01-25 18:22 ` Daniel Vetter
2024-01-25 21:55   ` Dave Airlie

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.