On Thu, 2010-05-27 at 17:55 +0300, Pekka Paalanen wrote: > On Wed, 26 May 2010 23:24:57 +0200 > Maarten Maathuis <madman2...@gmail.com> wrote: > > > For NV04 i can understand, since it's irq driven fences, so let's > > split the question. > > > > NV10+: can we reduce it to just spin_lock? > > I don't know the answer, but I know the theory: if there is > any path, that can take the spinlock from an interrupt > service path, then you must use the irq-safe version everywhere. We could, the interrupt-based path is currently only used on really old chips that don't have REF_CNT.
> > > NV04: can't we rely on a normal spin lock and add it as well in > > nv04_graph_mthd_set_ref? > > So if NV04 fences are driven by irqs, and the ISR needs to > take the lock, then no, you cannot revert to irq-unsafe spinlocks. > I'm not sure how it relates to ISR bottom halves, though. > > Note, that also irq-unsafe spinlocks disable preemption, which > might be enough to disturb audio. The spinlock was actually only ever meant to protect the list itself and not the sequence counters. I've attached a patch removing the spinlock use everywhere except when we're actually going to touch the pending list, I think last_sequence_irq is still safe as the NV04 fence IRQ handler is the only writer. I haven't tested beyond knowing this laptop I'm typing on still works. Thoughts? > > > my 2c >
diff --git a/drivers/gpu/drm/nouveau/nouveau_channel.c b/drivers/gpu/drm/nouveau/nouveau_channel.c index 1fc57ef..f9b2acf 100644 --- a/drivers/gpu/drm/nouveau/nouveau_channel.c +++ b/drivers/gpu/drm/nouveau/nouveau_channel.c @@ -257,9 +257,7 @@ nouveau_channel_free(struct nouveau_channel *chan) nouveau_debugfs_channel_fini(chan); /* Give outstanding push buffers a chance to complete */ - spin_lock_irqsave(&chan->fence.lock, flags); nouveau_fence_update(chan); - spin_unlock_irqrestore(&chan->fence.lock, flags); if (chan->fence.sequence != chan->fence.sequence_ack) { struct nouveau_fence *fence = NULL; diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c index faddf53..4031998 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fence.c +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c @@ -73,6 +73,7 @@ nouveau_fence_update(struct nouveau_channel *chan) return; chan->fence.sequence_ack = sequence; + spin_lock(&chan->fence.lock); list_for_each_safe(entry, tmp, &chan->fence.pending) { fence = list_entry(entry, struct nouveau_fence, entry); @@ -84,6 +85,7 @@ nouveau_fence_update(struct nouveau_channel *chan) if (sequence == chan->fence.sequence_ack) break; } + spin_unlock(&chan->fence.lock); } int @@ -119,7 +121,6 @@ nouveau_fence_emit(struct nouveau_fence *fence) { struct drm_nouveau_private *dev_priv = fence->channel->dev->dev_private; struct nouveau_channel *chan = fence->channel; - unsigned long flags; int ret; ret = RING_SPACE(chan, 2); @@ -127,9 +128,7 @@ nouveau_fence_emit(struct nouveau_fence *fence) return ret; if (unlikely(chan->fence.sequence == chan->fence.sequence_ack - 1)) { - spin_lock_irqsave(&chan->fence.lock, flags); nouveau_fence_update(chan); - spin_unlock_irqrestore(&chan->fence.lock, flags); BUG_ON(chan->fence.sequence == chan->fence.sequence_ack - 1); @@ -138,9 +137,9 @@ nouveau_fence_emit(struct nouveau_fence *fence) fence->sequence = ++chan->fence.sequence; kref_get(&fence->refcount); - spin_lock_irqsave(&chan->fence.lock, flags); + spin_lock(&chan->fence.lock); list_add_tail(&fence->entry, &chan->fence.pending); - spin_unlock_irqrestore(&chan->fence.lock, flags); + spin_unlock(&chan->fence.lock); BEGIN_RING(chan, NvSubSw, USE_REFCNT ? 0x0050 : 0x0150, 1); OUT_RING(chan, fence->sequence); @@ -173,14 +172,11 @@ nouveau_fence_signalled(void *sync_obj, void *sync_arg) { struct nouveau_fence *fence = nouveau_fence(sync_obj); struct nouveau_channel *chan = fence->channel; - unsigned long flags; if (fence->signalled) return true; - spin_lock_irqsave(&chan->fence.lock, flags); nouveau_fence_update(chan); - spin_unlock_irqrestore(&chan->fence.lock, flags); return fence->signalled; } @@ -230,11 +226,10 @@ nouveau_fence_handler(struct drm_device *dev, int channel) if (channel >= 0 && channel < dev_priv->engine.fifo.channels) chan = dev_priv->fifos[channel]; - if (chan) { - spin_lock_irq(&chan->fence.lock); - nouveau_fence_update(chan); - spin_unlock_irq(&chan->fence.lock); - } + if (!chan) + return; + + chan->fence.last_sequence_irq++; } int
_______________________________________________ Nouveau mailing list Nouveau@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/nouveau