[PATCH 08/10] drm/radeon: replace pflip and sw_int counters with atomics

[Michel Dänzer]

On Fre, 2012-06-01 at 12:44 +0200, Christian König wrote: 
> On 01.06.2012 08:30, Michel Dänzer wrote:
> > On Fre, 2012-06-01 at 08:19 +0200, Michel Dänzer wrote:
> >> I think this might introduce a race condition:
> >>
> >> Thread 0 Thread 1
> >> -------- --------
> >> atomic_inc_return() returns 1
> >> spin_lock_irqsave()
> >> atomic_dec_and_test()
> >> radeon_irq_set()
> >>
> >> =>  the interrupt won't be enabled.
> > Hrmm, I messed up the formatting there, let me try one more time:
> >
> > Thread 0				Thread 1
> > --------				--------
> > atomic_inc_return() returns 1
> > spin_lock_irqsave()
> > 					atomic_dec_and_test()
> > radeon_irq_set()
> >
> >
> Nope that isn't a problem, cause what you really get in your example is:
> 
> Thread 0				Thread 1
> --------				--------
> atomic_inc_return() returns 1
> spin_lock_irqsave()
> 					atomic_dec_and_test()
> radeon_irq_set()
> spin_unlock_irqrestore()
> 					spin_lock_irqsave()
> 					radeon_irq_set()
> 					spin_unlock_irqrestore()
> 
> 
> So testing the atomic counters just determines if we need an update of 
> the irq registers or not, and since a significant change will always 
> trigger an update we can make sure that the irq regs are always set to 
> the last known state. We might call radeon_irq_set more often than 
> necessary, but that won't hurt us and is really unlikely.

Yeah, I also realized in the meantime that the race can't happen. I
blame it on lack of caffeine. :)


> Also I have found the real reason why using the atomic for preventing ih 
> recursion didn't worked as expected - it was just a stupid typo in my 
> patch.  But thanks for the comment anyway, it got me to look into the 
> right direction for the bug.

Glad to hear that!


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer