[Nouveau] Why do we disable local IRQ around nouveau_fence_update?

[Ben Skeggs]

Sent from my iPhone

On 30/05/2010, at 9:22, Maarten Maathuis <madman2003 at gmail.com> wrote:

> On Fri, May 28, 2010 at 7:47 AM, Ben Skeggs <skeggsb at gmail.com> wrote:
>> On Thu, 2010-05-27 at 17:55 +0300, Pekka Paalanen wrote:
>>> On Wed, 26 May 2010 23:24:57 +0200
>>> Maarten Maathuis <madman2003 at 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?
>>
>
> Why the extra "chan->fence.last_sequence_irq++"? Isn't it already set
> from nv04_graph_mthd_set_ref?
Ah good catch, if I push this I'll fix that first :)

Ben.

>
>>>
>>>
>>> my 2c
>>>
>>
>>
>
>
>
> -- 
> Life spent, a precious moment, in the wink of an eye we live and we  
> die.